[HashRecognize] Fix LHS ConstantRange check for BE

The ConstantRange of the LHS of the ICmp in the Select(ICmp())
significant-bit check is not checked in the big-endian case, leading to
a potential miscompile. Fix this.

Co-authored-by: Piotr Fusik <[email protected]>

Author: artagnon

llvm/lib/Analysis/HashRecognize.cpp

@@ -90,6 +90,7 @@ class ValueEvolution {
   const bool ByteOrderSwapped;
   APInt GenPoly;
   StringRef ErrStr;
+  unsigned AtIter;
 
   // Compute the KnownBits of a BinaryOperator.
   KnownBits computeBinOp(const BinaryOperator *I);
@@ -190,43 +191,54 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I) {
     return KnownPhis.lookup_or(P, BitWidth);
 
   // Compute the KnownBits for a Select(Cmp()), forcing it to take the branch
-  // that is predicated on the (least|most)-significant-bit check.
+  // that is (least|most)-significant-bit-clear.
   CmpPredicate Pred;
   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)));
 
-    // 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) {
-      KnownBits KnownL = compute(L);
-      unsigned ICmpBW = KnownL.getBitWidth();
-      auto LCR = ConstantRange::fromKnownBits(KnownL, false);
-      auto CheckLCR = ConstantRange(APInt::getZero(ICmpBW), APInt(ICmpBW, 2));
-      if (LCR != CheckLCR) {
-        ErrStr = "Bad LHS of significant-bit-check";
-        return {BitWidth};
-      }
+    // Check that the predication is on (most|least) significant bit. We check
+    // that the compare is `>= 0` in the big-endian case, and `== 0` in the
+    // little-endian case (or the inverse, in which case the branches of the
+    // compare are swapped). We check LCR against CheckLCR, which is full-set,
+    // [0, -1), [0, -3), [0, -7), etc. depending on AtIter in the big-endian
+    // case, and [0, 2) in the little-endian case: CheckLCR checks that we are
+    // shifting in zero bits in every loop iteration in the big-endian case, and
+    // the compare 0 or 1 in the little-endian case (as a value and'ed with 1 is
+    // passed as the operand). We then check AllowedByR against CheckAllowedByR,
+    // which is [0, smin) in the big-endian case, and [0, 1) in the
+    // little-endian case: CheckAllowedByR checks for significant-bit-clear,
+    // which means that we visit the bit-shift branch instead of the
+    // bit-shift-and-xor-poly branch.
+    KnownBits KnownL = compute(L);
+    unsigned ICmpBW = KnownL.getBitWidth();
+    auto LCR = ConstantRange::fromKnownBits(KnownL, false);
+    auto CheckLCR = ConstantRange::getNonEmpty(
+        APInt::getZero(ICmpBW), ByteOrderSwapped
+                                    ? -APInt::getLowBitsSet(ICmpBW, AtIter)
+                                    : APInt(ICmpBW, 2));
+    if (LCR != CheckLCR) {
+      ErrStr = "Bad LHS of significant-bit-check";
+      return {BitWidth};
     }
 
-    // Check that the predication is on (most|least) significant bit.
     KnownBits KnownR = compute(R);
-    unsigned ICmpBW = KnownR.getBitWidth();
     auto RCR = ConstantRange::fromKnownBits(KnownR, false);
-    auto AllowedR = ConstantRange::makeAllowedICmpRegion(Pred, RCR);
-    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) {
+    auto AllowedByR = ConstantRange::makeAllowedICmpRegion(Pred, RCR);
+    ConstantRange CheckAllowedByR(
+        APInt::getZero(ICmpBW),
+        ByteOrderSwapped ? APInt::getSignedMinValue(ICmpBW) : APInt(ICmpBW, 1));
+
+    // We only compute KnownBits of either TV or FV, as the other branch would
+    // be the bit-shift-and-xor-poly branch, as determined by the conditional
+    // recurrence.
+    if (AllowedByR == CheckAllowedByR) {
       Visited.insert(FV);
       return compute(TV);
     }
-    if (AllowedR.inverse() == CheckRCR) {
+    if (AllowedByR.inverse() == CheckAllowedByR) {
       Visited.insert(TV);
       return compute(FV);
     }
@@ -264,9 +276,11 @@ KnownBits ValueEvolution::compute(const Value *V) {
 }
 
 bool ValueEvolution::computeEvolutions(ArrayRef<PhiStepPair> PhiEvolutions) {
-  for (unsigned I = 0; I < TripCount; ++I)
+  for (unsigned I = 0; I < TripCount; ++I) {
+    AtIter = I;
     for (auto [Phi, Step] : PhiEvolutions)
       KnownPhis.emplace_or_assign(Phi, computeInstr(Step));
+  }
 
   return ErrStr.empty();
 }

llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll

@@ -649,7 +649,7 @@ exit:                                              ; preds = %loop
 define i16 @not.crc.wrong.sb.check.const(i8 %msg, i16 %checksum) {
 ; CHECK-LABEL: 'not.crc.wrong.sb.check.const'
 ; CHECK-NEXT:  Did not find a hash algorithm
-; CHECK-NEXT:  Reason: Bad RHS of significant-bit-check
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
 ;
 entry:
   br label %loop
@@ -676,7 +676,7 @@ exit:                                              ; preds = %loop
 define i16 @not.crc.wrong.sb.check.pred(i16 %crc.init) {
 ; CHECK-LABEL: 'not.crc.wrong.sb.check.pred'
 ; CHECK-NEXT:  Did not find a hash algorithm
-; CHECK-NEXT:  Reason: Bad RHS of significant-bit-check
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
 ;
 entry:
   br label %loop
@@ -857,10 +857,10 @@ exit:                                              ; preds = %loop
   ret i16 %crc.next
 }
 
-define i16 @crc1.tc8.sb.check.endian.mismatch(i8 %msg, i16 %checksum) {
-; CHECK-LABEL: 'crc1.tc8.sb.check.endian.mismatch'
+define i16 @not.crc.sb.check.endian.mismatch(i8 %msg, i16 %checksum) {
+; CHECK-LABEL: 'not.crc.sb.check.endian.mismatch'
 ; CHECK-NEXT:  Did not find a hash algorithm
-; CHECK-NEXT:  Reason: Bad RHS of significant-bit-check
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
 ;
 entry:
   br label %loop
@@ -912,7 +912,7 @@ exit:                                              ; preds = %loop
 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: Found stray unvisited instructions
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
 ;
 entry:
   br label %loop
@@ -1219,7 +1219,7 @@ declare void @print(i16)
 define i16 @not.crc.call.sb.check(i16 %crc.init) {
 ; CHECK-LABEL: 'not.crc.call.sb.check'
 ; CHECK-NEXT:  Did not find a hash algorithm
-; CHECK-NEXT:  Reason: Found stray unvisited instructions
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
 ;
 entry:
   br label %loop
@@ -1241,3 +1241,26 @@ exit:                                              ; preds = %loop
 }
 
 declare i16 @side.effect()
+
+define i16 @not.crc.bad.lhs.sb.check.be(i16 %crc.init) {
+; CHECK-LABEL: 'not.crc.bad.lhs.sb.check.be'
+; CHECK-NEXT:  Did not find a hash algorithm
+; CHECK-NEXT:  Reason: Bad LHS of significant-bit-check
+;
+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.shl, 0
+  %crc.next = select i1 %check.sb, i16 %crc.xor, i16 %crc.shl
+  %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
+}