[HashRecognize] Address latest review

Author: artagnon

llvm/lib/Analysis/HashRecognize.cpp

@@ -182,33 +182,31 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I,
   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)))) {
-    KnownBits KnownL = compute(L, KnownPhis).zextOrTrunc(BitWidth);
-    KnownBits KnownR = compute(R, KnownPhis).zextOrTrunc(BitWidth);
-    KnownBits KnownTV = compute(TV, KnownPhis);
-    KnownBits KnownFV = compute(FV, KnownPhis);
-    auto LCR = ConstantRange::fromKnownBits(KnownL, false);
-    auto RCR = ConstantRange::fromKnownBits(KnownR, false);
-
     // We need to check LCR against [0, 2) in the little-endian case, because
     // the RCR check is insufficient: it is simply [0, 1).
-    auto CheckLCR = ConstantRange(APInt::getZero(BitWidth), APInt(BitWidth, 2));
-    if (!ByteOrderSwapped && LCR != CheckLCR) {
-      ErrStr = "Bad LHS of significant-bit-check";
-      return {BitWidth};
+    if (!ByteOrderSwapped) {
+      KnownBits KnownL = compute(L, KnownPhis).zextOrTrunc(BitWidth);
+      auto LCR = ConstantRange::fromKnownBits(KnownL, false);
+      auto CheckLCR =
+          ConstantRange(APInt::getZero(BitWidth), APInt(BitWidth, 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, KnownPhis).zextOrTrunc(BitWidth);
+    auto RCR = ConstantRange::fromKnownBits(KnownR, false);
     auto AllowedR = ConstantRange::makeAllowedICmpRegion(Pred, RCR);
-    auto InverseR = ConstantRange::makeAllowedICmpRegion(
-        CmpInst::getInversePredicate(Pred), RCR);
     ConstantRange LSBRange(APInt::getZero(BitWidth), APInt(BitWidth, 1));
     ConstantRange MSBRange(APInt::getZero(BitWidth),
                            APInt::getSignedMinValue(BitWidth));
     const ConstantRange &CheckRCR = ByteOrderSwapped ? MSBRange : LSBRange;
     if (AllowedR == CheckRCR)
-      return KnownTV;
+      return compute(TV, KnownPhis);
     if (AllowedR.inverse() == CheckRCR)
-      return KnownFV;
+      return compute(FV, KnownPhis);
 
     ErrStr = "Bad RHS of significant-bit-check";
     return {BitWidth};
@@ -233,9 +231,8 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I,
 /// Compute the KnownBits of Value \p V.
 KnownBits ValueEvolution::compute(const Value *V,
                                   const KnownPhiMap &KnownPhis) {
-  const APInt *C;
-  if (match(V, m_APInt(C)))
-    return KnownBits::makeConstant(*C);
+  if (auto *CI = dyn_cast<ConstantInt>(V))
+    return KnownBits::makeConstant(CI->getValue());
 
   if (auto *I = dyn_cast<Instruction>(V))
     return computeInstr(I, KnownPhis);
@@ -260,7 +257,7 @@ ValueEvolution::computeEvolutions(ArrayRef<PhiStepPair> PhiEvolutions) {
       KnownPhis.emplace_or_assign(Phi, KnownAtIter);
     }
   }
-  return KnownPhis;
+  return hasError() ? std::nullopt : std::make_optional(KnownPhis);
 }
 
 /// A structure that can hold either a Simple Recurrence or a Conditional
@@ -375,11 +372,11 @@ RecurrenceInfo::digRecurrence(Instruction *V,
 /// where %tv and %fv ultimately end up using %rec via the same %BO instruction,
 /// after digging through the use-def chain.
 ///
-/// \p ExtraConst is relevant if \p BOWithConstOpToMatch is supplied: when
-/// digging the use-def chain, a BinOp with opcode \p BOWithConstOpToMatch is
-/// matched, and \p ExtraConst is a constant operand of that BinOp. This
-/// peculiarity exists, because in a CRC algorithm, the \p BOWithConstOpToMatch
-/// is an XOR, and the \p ExtraConst ends up being the generating polynomial.
+/// ExtraConst is relevant if \p BOWithConstOpToMatch is supplied: when digging
+/// the use-def chain, a BinOp with opcode \p BOWithConstOpToMatch is matched,
+/// and ExtraConst is a constant operand of that BinOp. This peculiarity exists,
+/// because in a CRC algorithm, the \p BOWithConstOpToMatch is an XOR, and the
+/// ExtraConst ends up being the generating polynomial.
 bool RecurrenceInfo::matchConditionalRecurrence(
     const PHINode *P, Instruction::BinaryOps BOWithConstOpToMatch) {
   Phi = P;
@@ -447,9 +444,11 @@ PolynomialInfo::PolynomialInfo(unsigned TripCount, const Value *LHS,
     : TripCount(TripCount), LHS(LHS), RHS(RHS), ComputedValue(ComputedValue),
       ByteOrderSwapped(ByteOrderSwapped), LHSAux(LHSAux) {}
 
-/// In big-endian case, checks that the bottom N bits against CheckFn, and that
-/// the rest are unknown. In little-endian case, checks that the top N bits
-/// against CheckFn, and that the rest are unknown.
+/// In the big-endian case, checks the bottom N bits against CheckFn, and that
+/// the rest are unknown. In the little-endian case, checks the top N bits
+/// against CheckFn, and that the rest are unknown. Callers usually call this
+/// function with N = TripCount, and CheckFn checking that the remainder bits of
+/// the CRC polynomial division are zero.
 static bool checkExtractBits(const KnownBits &Known, unsigned N,
                              function_ref<bool(const KnownBits &)> CheckFn,
                              bool ByteOrderSwapped) {
@@ -555,7 +554,7 @@ HashRecognize::recognizeCRC() const {
   if (!L.isInnermost())
     return "Loop is not innermost";
   unsigned TC = SE.getSmallConstantMaxTripCount(&L);
-  if (!TC)
+  if (!TC || TC > 256)
     return "Unable to find a small constant trip count";
   BasicBlock *Latch = L.getLoopLatch();
   BasicBlock *Exit = L.getExitBlock();
@@ -600,7 +599,9 @@ HashRecognize::recognizeCRC() const {
   const APInt &GenPoly = *ConditionalRecurrence.ExtraConst;
 
   // PhiEvolutions are pairs of PHINodes along with their incoming value from
-  // within the loop, which we term as their step.
+  // within the loop, which we term as their step. Note that in the case of a
+  // Simple Recurrence, Step is an operand of the BO, while in a Conditional
+  // Recurrence, it is a SelectInst.
   SmallVector<PhiStepPair, 2> PhiEvolutions;
   PhiEvolutions.emplace_back(ConditionalRecurrence.Phi, ComputedValue);
   if (SimpleRecurrence)