From e001448ae2504879c20876927e83860115363a85 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 9 Jul 2025 19:08:00 +0100 Subject: [PATCH 1/3] [HashRecognize] Pre-commit test for stray-unvisited --- .../HashRecognize/cyclic-redundancy-check.ll | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index 247a105940e6e..add4efd0bfca8 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll @@ -1189,3 +1189,48 @@ loop: ; preds = %loop, %entry exit: ; preds = %loop ret i16 %crc.next } + +define i16 @not.crc.stray.unvisited.inst(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.stray.unvisited.inst' +; CHECK-NEXT: Found big-endian CRC-16 loop with trip count 8 +; CHECK-NEXT: Initial CRC: i16 %crc.init +; CHECK-NEXT: Generating polynomial: 4129 +; CHECK-NEXT: Computed CRC: %crc.next = select i1 %check.sb, i16 %crc.xor, i16 %crc.shl +; CHECK-NEXT: Computed CRC lookup table: +; CHECK-NEXT: 0 4129 8258 12387 16516 20645 24774 28903 33032 37161 41290 45419 49548 53677 57806 61935 +; CHECK-NEXT: 4657 528 12915 8786 21173 17044 29431 25302 37689 33560 45947 41818 54205 50076 62463 58334 +; CHECK-NEXT: 9314 13379 1056 5121 25830 29895 17572 21637 42346 46411 34088 38153 58862 62927 50604 54669 +; CHECK-NEXT: 13907 9842 5649 1584 30423 26358 22165 18100 46939 42874 38681 34616 63455 59390 55197 51132 +; CHECK-NEXT: 18628 22757 26758 30887 2112 6241 10242 14371 51660 55789 59790 63919 35144 39273 43274 47403 +; CHECK-NEXT: 23285 19156 31415 27286 6769 2640 14899 10770 56317 52188 64447 60318 39801 35672 47931 43802 +; CHECK-NEXT: 27814 31879 19684 23749 11298 15363 3168 7233 60846 64911 52716 56781 44330 48395 36200 40265 +; CHECK-NEXT: 32407 28342 24277 20212 15891 11826 7761 3696 65439 61374 57309 53244 48923 44858 40793 36728 +; CHECK-NEXT: 37256 33193 45514 41451 53516 49453 61774 57711 4224 161 12482 8419 20484 16421 28742 24679 +; CHECK-NEXT: 33721 37784 41979 46042 49981 54044 58239 62302 689 4752 8947 13010 16949 21012 25207 29270 +; CHECK-NEXT: 46570 42443 38312 34185 62830 58703 54572 50445 13538 9411 5280 1153 29798 25671 21540 17413 +; CHECK-NEXT: 42971 47098 34713 38840 59231 63358 50973 55100 9939 14066 1681 5808 26199 30326 17941 22068 +; CHECK-NEXT: 55628 51565 63758 59695 39368 35305 47498 43435 22596 18533 30726 26663 6336 2273 14466 10403 +; CHECK-NEXT: 52093 56156 60223 64286 35833 39896 43963 48026 19061 23124 27191 31254 2801 6864 10931 14994 +; CHECK-NEXT: 64814 60687 56684 52557 48554 44427 40424 36297 31782 27655 23652 19525 15522 11395 7392 3265 +; CHECK-NEXT: 61215 65342 53085 57212 44955 49082 36825 40952 28183 32310 20053 24180 11923 16050 3793 7920 +; +entry: + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ] + %crc = phi i16 [ %crc.init, %entry ], [ %crc.next, %loop ] + %crc.shl = shl i16 %crc, 1 + %crc.xor = xor i16 %crc.shl, 4129 + %check.sb = icmp slt i16 %crc, 0 + %crc.next = select i1 %check.sb, i16 %crc.xor, i16 %crc.shl + call void @print(i16 %crc.next) + %iv.next = add nuw nsw i32 %iv, 1 + %exit.cond = icmp samesign ult i32 %iv, 7 + br i1 %exit.cond, label %loop, label %exit + +exit: ; preds = %loop + ret i16 %crc.next +} + +declare void @print(i16) From 7bcbc04c1223ce3415f32dec1d5f9b269982c5e3 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 9 Jul 2025 20:29:13 +0100 Subject: [PATCH 2/3] [HashRecognize] Track visited in ValueEvolution Require that all Instructions in the Loop are visited by ValueEvolution, as any stray instructions would complicate life for the optimization. --- llvm/lib/Analysis/HashRecognize.cpp | 52 +++++++++++++++---- .../HashRecognize/cyclic-redundancy-check.ll | 33 +++--------- 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index 2cc3ad5f18482..f032593492287 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -91,6 +91,10 @@ class ValueEvolution { APInt GenPoly; StringRef ErrStr; + // A set of instructions visited by ValueEvolution. Anything that's not in the + // use-def chain of the PHIs' evolution will be reported as unvisited. + SmallPtrSet Visited; + // Compute the KnownBits of a BinaryOperator. KnownBits computeBinOp(const BinaryOperator *I); @@ -102,15 +106,19 @@ class ValueEvolution { public: // ValueEvolution is meant to be constructed with the TripCount of the loop, - // and whether the polynomial algorithm is big-endian, for the significant-bit - // check. - ValueEvolution(unsigned TripCount, bool ByteOrderSwapped); + // whether the polynomial algorithm is big-endian for the significant-bit + // check, and an initial value for the Visited set. + ValueEvolution(unsigned TripCount, bool ByteOrderSwapped, + ArrayRef InitVisited); // Given a list of PHI nodes along with their incoming value from within the // loop, computeEvolutions computes the KnownBits of each of the PHI nodes on // the final iteration. Returns true on success and false on error. bool computeEvolutions(ArrayRef PhiEvolutions); + // Query the Visited set. + bool isVisited(const Instruction *I) const { return Visited.contains(I); } + // In case ValueEvolution encounters an error, this is meant to be used for a // precise error message. StringRef getError() const { return ErrStr; } @@ -120,8 +128,11 @@ class ValueEvolution { KnownPhiMap KnownPhis; }; -ValueEvolution::ValueEvolution(unsigned TripCount, bool ByteOrderSwapped) - : TripCount(TripCount), ByteOrderSwapped(ByteOrderSwapped) {} +ValueEvolution::ValueEvolution(unsigned TripCount, bool ByteOrderSwapped, + ArrayRef InitVisited) + : TripCount(TripCount), ByteOrderSwapped(ByteOrderSwapped) { + Visited.insert_range(InitVisited); +} KnownBits ValueEvolution::computeBinOp(const BinaryOperator *I) { KnownBits KnownL(compute(I->getOperand(0))); @@ -177,6 +188,9 @@ KnownBits ValueEvolution::computeBinOp(const BinaryOperator *I) { KnownBits ValueEvolution::computeInstr(const Instruction *I) { unsigned BitWidth = I->getType()->getScalarSizeInBits(); + // computeInstr is the only entry-point that needs to update the Visited set. + Visited.insert(I); + // We look up in the map that contains the KnownBits of the PHI from the // previous iteration. if (const PHINode *P = dyn_cast(I)) @@ -185,9 +199,14 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I) { // Compute the KnownBits for a Select(Cmp()), forcing it to take the branch // that is predicated on the (least|most)-significant-bit check. CmpPredicate Pred; - Value *L, *R, *TV, *FV; - if (match(I, m_Select(m_ICmp(Pred, m_Value(L), m_Value(R)), m_Value(TV), - m_Value(FV)))) { + Value *L, *R; + Instruction *TV, *FV; + if (match(I, m_Select(m_ICmp(Pred, m_Value(L), m_Value(R)), m_Instruction(TV), + m_Instruction(FV)))) { + Visited.insert(cast(I->getOperand(0))); + Visited.insert(TV); + Visited.insert(FV); + // We need to check LCR against [0, 2) in the little-endian case, because // the RCR check is insufficient: it is simply [0, 1). if (!ByteOrderSwapped) { @@ -209,6 +228,9 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I) { ConstantRange CheckRCR(APInt::getZero(ICmpBW), ByteOrderSwapped ? APInt::getSignedMinValue(ICmpBW) : APInt(ICmpBW, 1)); + + // We only compute KnownBits of either TV or FV, as the other value would + // just be a bit-shift as checked by isBigEndianBitShift. if (AllowedR == CheckRCR) return compute(TV); if (AllowedR.inverse() == CheckRCR) @@ -629,11 +651,23 @@ HashRecognize::recognizeCRC() const { if (SimpleRecurrence) PhiEvolutions.emplace_back(SimpleRecurrence.Phi, SimpleRecurrence.BO); - ValueEvolution VE(TC, *ByteOrderSwapped); + // Initialize the Visited set in ValueEvolution with the IndVar-related + // instructions. + std::initializer_list InitVisited = { + IndVar, Latch->getTerminator(), L.getLatchCmpInst(), + cast(IndVar->getIncomingValueForBlock(Latch))}; + + ValueEvolution VE(TC, *ByteOrderSwapped, InitVisited); if (!VE.computeEvolutions(PhiEvolutions)) return VE.getError(); KnownBits ResultBits = VE.KnownPhis.at(ConditionalRecurrence.Phi); + // Any unvisited instructions from the KnownBits propagation can complicate + // the optimization, which would just replace the entire loop with the + // table-lookup version of the hash algorithm. + if (any_of(*Latch, [VE](const Instruction &I) { return !VE.isVisited(&I); })) + return "Found stray unvisited instructions"; + unsigned N = std::min(TC, ResultBits.getBitWidth()); auto IsZero = [](const KnownBits &K) { return K.isZero(); }; if (!checkExtractBits(ResultBits, N, IsZero, *ByteOrderSwapped)) diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index add4efd0bfca8..3926c467375ed 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll @@ -909,10 +909,10 @@ exit: ; preds = %loop ret i16 %crc.next } -define i16 @not.crc.bad.cast(i8 %msg, i16 %checksum) { -; CHECK-LABEL: 'not.crc.bad.cast' +define i16 @not.crc.bad.endian.swapped.sb.check(i8 %msg, i16 %checksum) { +; CHECK-LABEL: 'not.crc.bad.endian.swapped.sb.check' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Expected bottom 8 bits zero (????????00001011) +; CHECK-NEXT: Reason: Found stray unvisited instructions ; entry: br label %loop @@ -1190,29 +1190,10 @@ exit: ; preds = %loop ret i16 %crc.next } -define i16 @not.crc.stray.unvisited.inst(i16 %crc.init) { -; CHECK-LABEL: 'not.crc.stray.unvisited.inst' -; CHECK-NEXT: Found big-endian CRC-16 loop with trip count 8 -; CHECK-NEXT: Initial CRC: i16 %crc.init -; CHECK-NEXT: Generating polynomial: 4129 -; CHECK-NEXT: Computed CRC: %crc.next = select i1 %check.sb, i16 %crc.xor, i16 %crc.shl -; CHECK-NEXT: Computed CRC lookup table: -; CHECK-NEXT: 0 4129 8258 12387 16516 20645 24774 28903 33032 37161 41290 45419 49548 53677 57806 61935 -; CHECK-NEXT: 4657 528 12915 8786 21173 17044 29431 25302 37689 33560 45947 41818 54205 50076 62463 58334 -; CHECK-NEXT: 9314 13379 1056 5121 25830 29895 17572 21637 42346 46411 34088 38153 58862 62927 50604 54669 -; CHECK-NEXT: 13907 9842 5649 1584 30423 26358 22165 18100 46939 42874 38681 34616 63455 59390 55197 51132 -; CHECK-NEXT: 18628 22757 26758 30887 2112 6241 10242 14371 51660 55789 59790 63919 35144 39273 43274 47403 -; CHECK-NEXT: 23285 19156 31415 27286 6769 2640 14899 10770 56317 52188 64447 60318 39801 35672 47931 43802 -; CHECK-NEXT: 27814 31879 19684 23749 11298 15363 3168 7233 60846 64911 52716 56781 44330 48395 36200 40265 -; CHECK-NEXT: 32407 28342 24277 20212 15891 11826 7761 3696 65439 61374 57309 53244 48923 44858 40793 36728 -; CHECK-NEXT: 37256 33193 45514 41451 53516 49453 61774 57711 4224 161 12482 8419 20484 16421 28742 24679 -; CHECK-NEXT: 33721 37784 41979 46042 49981 54044 58239 62302 689 4752 8947 13010 16949 21012 25207 29270 -; CHECK-NEXT: 46570 42443 38312 34185 62830 58703 54572 50445 13538 9411 5280 1153 29798 25671 21540 17413 -; CHECK-NEXT: 42971 47098 34713 38840 59231 63358 50973 55100 9939 14066 1681 5808 26199 30326 17941 22068 -; CHECK-NEXT: 55628 51565 63758 59695 39368 35305 47498 43435 22596 18533 30726 26663 6336 2273 14466 10403 -; CHECK-NEXT: 52093 56156 60223 64286 35833 39896 43963 48026 19061 23124 27191 31254 2801 6864 10931 14994 -; CHECK-NEXT: 64814 60687 56684 52557 48554 44427 40424 36297 31782 27655 23652 19525 15522 11395 7392 3265 -; CHECK-NEXT: 61215 65342 53085 57212 44955 49082 36825 40952 28183 32310 20053 24180 11923 16050 3793 7920 +define i16 @not.crc.stray.unvisited.call(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.stray.unvisited.call' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Found stray unvisited instructions ; entry: br label %loop From 6ec6823e8d64a5eb332be2001366fc0a6ee9bc50 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 10 Jul 2025 11:12:45 +0100 Subject: [PATCH 3/3] [HashRecognize] Minor simplification (NFC) --- llvm/lib/Analysis/HashRecognize.cpp | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index f032593492287..662260a79ab7a 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -91,10 +91,6 @@ class ValueEvolution { APInt GenPoly; StringRef ErrStr; - // A set of instructions visited by ValueEvolution. Anything that's not in the - // use-def chain of the PHIs' evolution will be reported as unvisited. - SmallPtrSet Visited; - // Compute the KnownBits of a BinaryOperator. KnownBits computeBinOp(const BinaryOperator *I); @@ -116,13 +112,14 @@ class ValueEvolution { // the final iteration. Returns true on success and false on error. bool computeEvolutions(ArrayRef PhiEvolutions); - // Query the Visited set. - bool isVisited(const Instruction *I) const { return Visited.contains(I); } - // In case ValueEvolution encounters an error, this is meant to be used for a // precise error message. StringRef getError() const { return ErrStr; } + // A set of instructions visited by ValueEvolution. Anything that's not in the + // use-def chain of the PHIs' evolution will not be visited. + SmallPtrSet Visited; + // The computed KnownBits for each PHI node, which is populated after // computeEvolutions is called. KnownPhiMap KnownPhis; @@ -130,9 +127,8 @@ class ValueEvolution { ValueEvolution::ValueEvolution(unsigned TripCount, bool ByteOrderSwapped, ArrayRef InitVisited) - : TripCount(TripCount), ByteOrderSwapped(ByteOrderSwapped) { - Visited.insert_range(InitVisited); -} + : TripCount(TripCount), ByteOrderSwapped(ByteOrderSwapped), + Visited(InitVisited.begin(), InitVisited.end()) {} KnownBits ValueEvolution::computeBinOp(const BinaryOperator *I) { KnownBits KnownL(compute(I->getOperand(0))); @@ -665,7 +661,7 @@ HashRecognize::recognizeCRC() const { // Any unvisited instructions from the KnownBits propagation can complicate // the optimization, which would just replace the entire loop with the // table-lookup version of the hash algorithm. - if (any_of(*Latch, [VE](const Instruction &I) { return !VE.isVisited(&I); })) + if (std::distance(Latch->begin(), Latch->end()) != VE.Visited.size()) return "Found stray unvisited instructions"; unsigned N = std::min(TC, ResultBits.getBitWidth());