[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.

Author: artagnon

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<const Instruction *, 16> 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<const Instruction *> 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<PhiStepPair> 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<const Instruction *> 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<PHINode>(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<Instruction>(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<const Instruction *> InitVisited = {
+      IndVar, Latch->getTerminator(), L.getLatchCmpInst(),
+      cast<Instruction>(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))

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