From 56e6c37397e235b911fdef667e06fd64ac96abb1 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Mon, 14 Jul 2025 12:20:35 +0100 Subject: [PATCH 1/6] [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 --- llvm/lib/Analysis/HashRecognize.cpp | 62 ++++++++++++------- .../HashRecognize/cyclic-redundancy-check.ll | 37 ++++++++--- 2 files changed, 68 insertions(+), 31 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index 92c9e37dbb484..99ca3cf75b666 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/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,7 +191,7 @@ 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; @@ -198,35 +199,46 @@ KnownBits ValueEvolution::computeInstr(const Instruction *I) { m_Instruction(FV)))) { Visited.insert(cast(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 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(); } diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index fe140d01e8818..8ebe6eaaaf0be 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/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 +} From 0e78eaedebfd7e09ef4102e965ecb01f33e563bf Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Tue, 5 Aug 2025 11:01:04 +0100 Subject: [PATCH 2/6] [HashRecognize] Strip ValueEvolution Co-authored-by: Piotr Fusik --- llvm/include/llvm/Analysis/HashRecognize.h | 8 +- llvm/lib/Analysis/HashRecognize.cpp | 347 +++++------------- .../HashRecognize/cyclic-redundancy-check.ll | 140 ++++++- 3 files changed, 211 insertions(+), 284 deletions(-) diff --git a/llvm/include/llvm/Analysis/HashRecognize.h b/llvm/include/llvm/Analysis/HashRecognize.h index 0361dfcd23528..6dea3d24885ff 100644 --- a/llvm/include/llvm/Analysis/HashRecognize.h +++ b/llvm/include/llvm/Analysis/HashRecognize.h @@ -20,18 +20,12 @@ #include "llvm/Analysis/ScalarEvolution.h" #include "llvm/IR/PassManager.h" #include "llvm/IR/Value.h" -#include "llvm/Support/KnownBits.h" #include namespace llvm { class LPMUpdater; -/// A tuple of bits that are expected to be zero, number N of them expected to -/// be zero, with a boolean indicating whether it's the top or bottom N bits -/// expected to be zero. -using ErrBits = std::tuple; - /// A custom std::array with 256 entries, that also has a print function. struct CRCTable : public std::array { void print(raw_ostream &OS) const; @@ -85,7 +79,7 @@ class HashRecognize { HashRecognize(const Loop &L, ScalarEvolution &SE); // The main analysis entry points. - std::variant recognizeCRC() const; + std::variant recognizeCRC() const; std::optional getResult() const; // Auxilary entry point after analysis to interleave the generating polynomial diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index 99ca3cf75b666..49fb07aa51a5c 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -73,216 +73,86 @@ using namespace SCEVPatternMatch; #define DEBUG_TYPE "hash-recognize" -// KnownBits for a PHI node. There are at most two PHI nodes, corresponding to -// the Simple Recurrence and Conditional Recurrence. The IndVar PHI is not -// relevant. -using KnownPhiMap = SmallDenseMap; - -// A pair of a PHI node along with its incoming value from within a loop. -using PhiStepPair = std::pair; - -/// A much simpler version of ValueTracking, in that it computes KnownBits of -/// values, except that it computes the evolution of KnownBits in a loop with a -/// given trip count, and predication is specialized for a significant-bit -/// check. -class ValueEvolution { - const unsigned TripCount; - const bool ByteOrderSwapped; - APInt GenPoly; - StringRef ErrStr; - unsigned AtIter; - - // Compute the KnownBits of a BinaryOperator. - KnownBits computeBinOp(const BinaryOperator *I); - - // Compute the KnownBits of an Instruction. - KnownBits computeInstr(const Instruction *I); - - // Compute the KnownBits of a Value. - KnownBits compute(const Value *V); - -public: - // ValueEvolution is meant to be constructed with the TripCount of the loop, - // and a boolean indicating whether the polynomial algorithm is big-endian - // (for the significant-bit check). - ValueEvolution(unsigned TripCount, bool ByteOrderSwapped); - - // 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); - - // 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. The only unvisited - // instructions will be ones not on the use-def chain of the PHIs' evolutions. - SmallPtrSet Visited; - - // The computed KnownBits for each PHI node, which is populated after - // computeEvolutions is called. - KnownPhiMap KnownPhis; -}; - -ValueEvolution::ValueEvolution(unsigned TripCount, bool ByteOrderSwapped) - : TripCount(TripCount), ByteOrderSwapped(ByteOrderSwapped) {} - -KnownBits ValueEvolution::computeBinOp(const BinaryOperator *I) { - KnownBits KnownL(compute(I->getOperand(0))); - KnownBits KnownR(compute(I->getOperand(1))); - - switch (I->getOpcode()) { - case Instruction::BinaryOps::And: - return KnownL & KnownR; - case Instruction::BinaryOps::Or: - return KnownL | KnownR; - case Instruction::BinaryOps::Xor: - return KnownL ^ KnownR; - case Instruction::BinaryOps::Shl: { - auto *OBO = cast(I); - return KnownBits::shl(KnownL, KnownR, OBO->hasNoUnsignedWrap(), - OBO->hasNoSignedWrap()); - } - case Instruction::BinaryOps::LShr: - return KnownBits::lshr(KnownL, KnownR); - case Instruction::BinaryOps::AShr: - return KnownBits::ashr(KnownL, KnownR); - case Instruction::BinaryOps::Add: { - auto *OBO = cast(I); - return KnownBits::add(KnownL, KnownR, OBO->hasNoUnsignedWrap(), - OBO->hasNoSignedWrap()); - } - case Instruction::BinaryOps::Sub: { - auto *OBO = cast(I); - return KnownBits::sub(KnownL, KnownR, OBO->hasNoUnsignedWrap(), - OBO->hasNoSignedWrap()); - } - case Instruction::BinaryOps::Mul: { - Value *Op0 = I->getOperand(0); - Value *Op1 = I->getOperand(1); - bool SelfMultiply = Op0 == Op1 && isGuaranteedNotToBeUndef(Op0); - return KnownBits::mul(KnownL, KnownR, SelfMultiply); - } - case Instruction::BinaryOps::UDiv: - return KnownBits::udiv(KnownL, KnownR); - case Instruction::BinaryOps::SDiv: - return KnownBits::sdiv(KnownL, KnownR); - case Instruction::BinaryOps::URem: - return KnownBits::urem(KnownL, KnownR); - case Instruction::BinaryOps::SRem: - return KnownBits::srem(KnownL, KnownR); - default: - ErrStr = "Unknown BinaryOperator"; - unsigned BitWidth = I->getType()->getScalarSizeInBits(); - return {BitWidth}; - } -} - -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)) - return KnownPhis.lookup_or(P, BitWidth); - - // Compute the KnownBits for a Select(Cmp()), forcing it to take the branch - // that is (least|most)-significant-bit-clear. +/// Check the well-formedness of the (most|least) significant bit check \p SI, +/// where \p BitShift is the bit-shift branch: the other branch must be a +/// bit-shift-and-xor-poly branch, as already checked by +/// matchConditionalRecurrence. 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 in the big-endian case, and [0, 2) in the +/// little-endian case: CheckLCR checks that the comparison is `>= 0` in the +/// big-endian case, and that the compare is to 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, and this must be equal to \p BitShift for +/// well-formedness. +static bool isSignificantBitCheckWellFormed(const SelectInst *SI, + const BinaryOperator *BitShift, + bool ByteOrderSwapped) { + DataLayout DL = SI->getParent()->getDataLayout(); 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(I->getOperand(0))); - - // 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}; - } - - KnownBits KnownR = compute(R); - auto RCR = ConstantRange::fromKnownBits(KnownR, false); - 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 (AllowedByR.inverse() == CheckAllowedByR) { - Visited.insert(TV); - return compute(FV); - } - - ErrStr = "Bad RHS of significant-bit-check"; - return {BitWidth}; - } + [[maybe_unused]] bool Match = + match(SI, m_Select(m_ICmp(Pred, m_Value(L), m_Value(R)), + m_Instruction(TV), m_Instruction(FV))); + assert(Match && "Select(ICmp()) expected in signficant-bit-check"); + + KnownBits KnownL = computeKnownBits(L, DL); + unsigned ICmpBW = KnownL.getBitWidth(); + auto LCR = ConstantRange::fromKnownBits(KnownL, false); + auto CheckLCR = ConstantRange::getNonEmpty( + APInt::getZero(ICmpBW), + ByteOrderSwapped ? APInt::getZero(ICmpBW) : APInt(ICmpBW, 2)); + if (LCR != CheckLCR) + return false; - if (auto *BO = dyn_cast(I)) - return computeBinOp(BO); - - switch (I->getOpcode()) { - case Instruction::CastOps::Trunc: - return compute(I->getOperand(0)).trunc(BitWidth); - case Instruction::CastOps::ZExt: - return compute(I->getOperand(0)).zext(BitWidth); - case Instruction::CastOps::SExt: - return compute(I->getOperand(0)).sext(BitWidth); - default: - ErrStr = "Unknown Instruction"; - return {BitWidth}; - } + KnownBits KnownR = computeKnownBits(R, DL); + auto RCR = ConstantRange::fromKnownBits(KnownR, false); + auto AllowedByR = ConstantRange::makeAllowedICmpRegion(Pred, RCR); + ConstantRange CheckAllowedByR( + APInt::getZero(ICmpBW), + ByteOrderSwapped ? APInt::getSignedMinValue(ICmpBW) : APInt(ICmpBW, 1)); + + if (AllowedByR == CheckAllowedByR) + return TV == BitShift; + if (AllowedByR.inverse() == CheckAllowedByR) + return FV == BitShift; + return false; } -KnownBits ValueEvolution::compute(const Value *V) { - if (auto *CI = dyn_cast(V)) - return KnownBits::makeConstant(CI->getValue()); +/// Checks if Loop \p L contains instructions unreachable or unhandled from \p +/// Roots on the use-def chain. +static bool +containsUnreachableOrUnhandled(const Loop &L, + ArrayRef Roots) { + SmallPtrSet Visited; + BasicBlock *Latch = L.getLoopLatch(); - if (auto *I = dyn_cast(V)) - return computeInstr(I); + SmallVector Worklist(Roots); + while (!Worklist.empty()) { + const Instruction *I = Worklist.pop_back_val(); + Visited.insert(I); - ErrStr = "Unknown Value"; - unsigned BitWidth = V->getType()->getScalarSizeInBits(); - return {BitWidth}; -} + if (isa(I)) + continue; -bool ValueEvolution::computeEvolutions(ArrayRef PhiEvolutions) { - for (unsigned I = 0; I < TripCount; ++I) { - AtIter = I; - for (auto [Phi, Step] : PhiEvolutions) - KnownPhis.emplace_or_assign(Phi, computeInstr(Step)); - } + if (!isa(I)) + return true; - return ErrStr.empty(); + for (const Use &U : I->operands()) { + if (auto *UI = dyn_cast(U)) { + if (!L.contains(UI)) + return true; + Worklist.push_back(UI); + continue; + } + if (!isa(U)) + return true; + } + } + return std::distance(Latch->begin(), Latch->end()) != Visited.size(); } /// A structure that can hold either a Simple Recurrence or a Conditional @@ -473,26 +343,6 @@ PolynomialInfo::PolynomialInfo(unsigned TripCount, Value *LHS, const APInt &RHS, : TripCount(TripCount), LHS(LHS), RHS(RHS), ComputedValue(ComputedValue), ByteOrderSwapped(ByteOrderSwapped), LHSAux(LHSAux) {} -/// 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 CheckFn, - bool ByteOrderSwapped) { - // Check that the entire thing is a constant. - if (N == Known.getBitWidth()) - return CheckFn(Known.extractBits(N, 0)); - - // Check that the {top, bottom} N bits are not unknown and that the {bottom, - // top} N bits are known. - unsigned BitPos = ByteOrderSwapped ? 0 : Known.getBitWidth() - N; - unsigned SwappedBitPos = ByteOrderSwapped ? N : 0; - return CheckFn(Known.extractBits(N, BitPos)) && - Known.extractBits(Known.getBitWidth() - N, SwappedBitPos).isUnknown(); -} - /// Generate a lookup table of 256 entries by interleaving the generating /// polynomial. The optimization technique of table-lookup for CRC is also /// called the Sarwate algorithm. @@ -525,9 +375,7 @@ CRCTable HashRecognize::genSarwateTable(const APInt &GenPoly, /// Checks that \p P1 and \p P2 are used together in an XOR in the use-def chain /// of \p SI's condition, ignoring any casts. The purpose of this function is to /// ensure that LHSAux from the SimpleRecurrence is used correctly in the CRC -/// computation. We cannot check the correctness of casts at this point, and -/// rely on the KnownBits propagation to check correctness of the CRC -/// computation. +/// computation. We cannot check the correctness of casts at this point. /// /// In other words, it checks for the following pattern: /// @@ -584,10 +432,8 @@ static std::optional isBigEndianBitShift(Value *V, ScalarEvolution &SE) { } /// The main entry point for analyzing a loop and recognizing the CRC algorithm. -/// Returns a PolynomialInfo on success, and either an ErrBits or a StringRef on -/// failure. -std::variant -HashRecognize::recognizeCRC() const { +/// Returns a PolynomialInfo on success, and a StringRef on failure. +std::variant HashRecognize::recognizeCRC() const { if (!L.isInnermost()) return "Loop is not innermost"; BasicBlock *Latch = L.getLoopLatch(); @@ -651,35 +497,19 @@ HashRecognize::recognizeCRC() const { "Expected ExtraConst in conditional recurrence"); 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. 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 PhiEvolutions; - PhiEvolutions.emplace_back(ConditionalRecurrence.Phi, ComputedValue); + if (!isSignificantBitCheckWellFormed( + cast(ConditionalRecurrence.Step), + ConditionalRecurrence.BO, *ByteOrderSwapped)) + return "Malformed significant-bit check"; + + SmallVector Roots( + {ComputedValue, + cast(IndVar->getIncomingValueForBlock(Latch)), + L.getLatchCmpInst(), Latch->getTerminator()}); if (SimpleRecurrence) - PhiEvolutions.emplace_back(SimpleRecurrence.Phi, SimpleRecurrence.BO); - - ValueEvolution VE(TC, *ByteOrderSwapped); - if (!VE.computeEvolutions(PhiEvolutions)) - return VE.getError(); - KnownBits ResultBits = VE.KnownPhis.at(ConditionalRecurrence.Phi); - - // There must be exactly four unvisited instructions, corresponding to the - // IndVar PHI. Any other unvisited instructions from the KnownBits propagation - // can complicate the optimization, which replaces the entire loop with the - // table-lookup version of the hash algorithm. - std::initializer_list AugmentVisited = { - IndVar, Latch->getTerminator(), L.getLatchCmpInst(), - cast(IndVar->getIncomingValueForBlock(Latch))}; - VE.Visited.insert_range(AugmentVisited); - if (std::distance(Latch->begin(), Latch->end()) != VE.Visited.size()) - 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)) - return ErrBits(ResultBits, TC, *ByteOrderSwapped); + Roots.push_back(SimpleRecurrence.BO); + if (containsUnreachableOrUnhandled(L, Roots)) + return "Found stray unvisited or unhandled instructions"; return PolynomialInfo(TC, LHS, GenPoly, ComputedValue, *ByteOrderSwapped, LHSAux); @@ -707,13 +537,6 @@ void HashRecognize::print(raw_ostream &OS) const { OS << "Did not find a hash algorithm\n"; if (std::holds_alternative(Ret)) OS << "Reason: " << std::get(Ret) << "\n"; - if (std::holds_alternative(Ret)) { - auto [Actual, Iter, ByteOrderSwapped] = std::get(Ret); - OS << "Reason: Expected " << (ByteOrderSwapped ? "bottom " : "top ") - << Iter << " bits zero ("; - Actual.print(OS); - OS << ")\n"; - } return; } diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index 8ebe6eaaaf0be..48464e69841f1 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/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 LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed 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 LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -750,7 +750,7 @@ exit: ; preds = %loop define i32 @not.crc.unknown.icmp.rhs(i32 %checksum, i32 %msg, i32 %unknown) { ; CHECK-LABEL: 'not.crc.unknown.icmp.rhs' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Bad LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -777,7 +777,7 @@ exit: ; preds = %loop define i32 @not.crc.unknown.icmp.lhs(i32 %checksum, i32 %msg, i32 %unknown) { ; CHECK-LABEL: 'not.crc.unknown.icmp.lhs' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Bad LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -805,7 +805,7 @@ exit: ; preds = %loop define i16 @not.crc.stray.or(i16 %msg, i16 %checksum) { ; CHECK-LABEL: 'not.crc.stray.or' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Bad LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -833,7 +833,7 @@ exit: ; preds = %loop define i16 @not.crc.inverse.sb.check(i16 %msg, i16 %checksum) { ; CHECK-LABEL: 'not.crc.inverse.sb.check' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Expected top 16 bits zero (1100000000000001) +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -860,7 +860,7 @@ exit: ; preds = %loop 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 LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -888,7 +888,7 @@ exit: ; preds = %loop define i16 @not.crc.init.arg.inverted.select(i16 %crc.init) { ; CHECK-LABEL: 'not.crc.init.arg.inverted.select' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Expected top 8 bits zero (11000000????????) +; CHECK-NEXT: Reason: Malformed 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: Bad LHS of significant-bit-check +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -1109,7 +1109,7 @@ exit: ; preds = %loop define i16 @not.crc.unknown.value(i16 %msg, i16 %checksum, i16 %corrupt) { ; CHECK-LABEL: 'not.crc.unknown.value' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Unknown Value +; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions ; entry: br label %loop @@ -1134,6 +1134,63 @@ exit: ; preds = %loop ret i16 %crc.next } +define i16 @not.crc.unknown.call.outside.loop(i16 %msg, i16 %checksum) { +; CHECK-LABEL: 'not.crc.unknown.call.outside.loop' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions +; +entry: + %corrupt = call i16 @side.effect() + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i8 [ 0, %entry ], [ %iv.next, %loop ] + %crc = phi i16 [ %checksum, %entry ], [ %crc.next, %loop ] + %data = phi i16 [ %msg, %entry ], [ %data.next, %loop ] + %xor.crc.data = xor i16 %crc, %data + %xor.crc.data.corrupt = mul i16 %xor.crc.data, %corrupt + %and.crc.data = and i16 %xor.crc.data.corrupt, 1 + %data.next = lshr i16 %data, 1 + %check.sb = icmp eq i16 %and.crc.data, 0 + %crc.lshr = lshr i16 %crc, 1 + %crc.xor = xor i16 %crc.lshr, -24575 + %crc.next = select i1 %check.sb, i16 %crc.lshr, i16 %crc.xor + %iv.next = add nuw nsw i8 %iv, 1 + %exit.cond = icmp samesign ult i8 %iv, 15 + br i1 %exit.cond, label %loop, label %exit + +exit: ; preds = %loop + ret i16 %crc.next +} + +define i16 @not.crc.constant.sb.check.corruption(i16 %msg, i16 %checksum) { +; CHECK-LABEL: 'not.crc.constant.sb.check.corruption' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Malformed significant-bit check +; +entry: + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i8 [ 0, %entry ], [ %iv.next, %loop ] + %crc = phi i16 [ %checksum, %entry ], [ %crc.next, %loop ] + %data = phi i16 [ %msg, %entry ], [ %data.next, %loop ] + %xor.crc.data = xor i16 %crc, %data + %xor.crc.data.corrupt = mul i16 %xor.crc.data, 2 + %and.crc.data = and i16 %xor.crc.data.corrupt, 1 + %data.next = lshr i16 %data, 1 + %check.sb = icmp eq i16 %and.crc.data, 0 + %crc.lshr = lshr i16 %crc, 1 + %crc.xor = xor i16 %crc.lshr, -24575 + %crc.next = select i1 %check.sb, i16 %crc.lshr, i16 %crc.xor + %iv.next = add nuw nsw i8 %iv, 1 + %exit.cond = icmp samesign ult i8 %iv, 15 + br i1 %exit.cond, label %loop, label %exit + +exit: ; preds = %loop + ret i16 %crc.next +} + define i16 @not.crc.float.simple.recurrence(float %msg, i16 %checksum) { ; CHECK-LABEL: 'not.crc.float.simple.recurrence' ; CHECK-NEXT: Did not find a hash algorithm @@ -1193,7 +1250,7 @@ exit: ; preds = %loop 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 +; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions ; entry: br label %loop @@ -1219,7 +1276,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: Bad LHS of significant-bit-check +; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions ; entry: br label %loop @@ -1240,12 +1297,10 @@ exit: ; preds = %loop ret i16 %crc.next } -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 +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -1264,3 +1319,58 @@ loop: ; preds = %loop, %entry exit: ; preds = %loop ret i16 %crc.next } + +define i16 @not.crc.knownbits.sb.check.fail(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.knownbits.sb.check.fail' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Malformed significant-bit check +; +entry: + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i16 [ 0, %entry ], [ %iv.next, %loop ] + %crc = phi i16 [ %crc.init, %entry ], [ %crc.next, %loop ] + %crc.shl = shl i16 %crc, 1 + %evil.and.iv = and i16 %iv, 2 + %evil.and.1 = add i16 %evil.and.iv, 1 + %evil.mul = mul i16 %crc.shl, %evil.and.1 + %evil.xor = xor i16 %evil.mul, 4129 + %check.sb = icmp slt i16 %crc, 0 + %crc.next = select i1 %check.sb, i16 %evil.xor, i16 %evil.mul + %iv.next = add nuw nsw i16 %iv, 1 + %exitcond.not = icmp eq i16 %iv.next, 8 + br i1 %exitcond.not, label %exit, label %loop + +exit: ; preds = %loop + ret i16 %crc.next +} + +define i16 @not.crc.knownbits.sb.check.fail.call.outside.loop(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.knownbits.sb.check.fail.call.outside.loop' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Malformed significant-bit check +; +entry: + %corrupt = call i16 @side.effect() + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i16 [ 0, %entry ], [ %iv.next, %loop ] + %crc = phi i16 [ %crc.init, %entry ], [ %crc.next, %loop ] + %crc.shl = shl i16 %crc, 1 + %evil.and.corrupt = and i16 %corrupt, 2 + %evil.and.1 = add i16 %evil.and.corrupt, 1 + %evil.mul = mul i16 %crc.shl, %evil.and.1 + %evil.xor = xor i16 %evil.mul, 4129 + %check.sb = icmp slt i16 %crc, 0 + %crc.next = select i1 %check.sb, i16 %evil.xor, i16 %evil.mul + %iv.next = add nuw nsw i16 %iv, 1 + %exitcond.not = icmp eq i16 %iv.next, 8 + br i1 %exitcond.not, label %exit, label %loop + +exit: ; preds = %loop + ret i16 %crc.next +} + +declare i16 @side.effect() From 4e8df0965faef0a3314983faf4ced73b49461cb6 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Mon, 1 Sep 2025 11:35:06 +0100 Subject: [PATCH 3/6] [HashRecognize] Match sb-check strongly --- llvm/lib/Analysis/HashRecognize.cpp | 128 +++++++++--------- .../HashRecognize/cyclic-redundancy-check.ll | 16 +-- 2 files changed, 71 insertions(+), 73 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index 49fb07aa51a5c..a8e480a78fde5 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -73,60 +73,10 @@ using namespace SCEVPatternMatch; #define DEBUG_TYPE "hash-recognize" -/// Check the well-formedness of the (most|least) significant bit check \p SI, -/// where \p BitShift is the bit-shift branch: the other branch must be a -/// bit-shift-and-xor-poly branch, as already checked by -/// matchConditionalRecurrence. 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 in the big-endian case, and [0, 2) in the -/// little-endian case: CheckLCR checks that the comparison is `>= 0` in the -/// big-endian case, and that the compare is to 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, and this must be equal to \p BitShift for -/// well-formedness. -static bool isSignificantBitCheckWellFormed(const SelectInst *SI, - const BinaryOperator *BitShift, - bool ByteOrderSwapped) { - DataLayout DL = SI->getParent()->getDataLayout(); - CmpPredicate Pred; - Value *L, *R; - Instruction *TV, *FV; - [[maybe_unused]] bool Match = - match(SI, m_Select(m_ICmp(Pred, m_Value(L), m_Value(R)), - m_Instruction(TV), m_Instruction(FV))); - assert(Match && "Select(ICmp()) expected in signficant-bit-check"); - - KnownBits KnownL = computeKnownBits(L, DL); - unsigned ICmpBW = KnownL.getBitWidth(); - auto LCR = ConstantRange::fromKnownBits(KnownL, false); - auto CheckLCR = ConstantRange::getNonEmpty( - APInt::getZero(ICmpBW), - ByteOrderSwapped ? APInt::getZero(ICmpBW) : APInt(ICmpBW, 2)); - if (LCR != CheckLCR) - return false; - - KnownBits KnownR = computeKnownBits(R, DL); - auto RCR = ConstantRange::fromKnownBits(KnownR, false); - auto AllowedByR = ConstantRange::makeAllowedICmpRegion(Pred, RCR); - ConstantRange CheckAllowedByR( - APInt::getZero(ICmpBW), - ByteOrderSwapped ? APInt::getSignedMinValue(ICmpBW) : APInt(ICmpBW, 1)); - - if (AllowedByR == CheckAllowedByR) - return TV == BitShift; - if (AllowedByR.inverse() == CheckAllowedByR) - return FV == BitShift; - return false; -} - -/// Checks if Loop \p L contains instructions unreachable or unhandled from \p -/// Roots on the use-def chain. -static bool -containsUnreachableOrUnhandled(const Loop &L, - ArrayRef Roots) { +/// Checks if Loop \p L contains instructions unreachable \p Roots on the +/// use-def chain. +static bool containsUnreachable(const Loop &L, + ArrayRef Roots) { SmallPtrSet Visited; BasicBlock *Latch = L.getLoopLatch(); @@ -138,9 +88,6 @@ containsUnreachableOrUnhandled(const Loop &L, if (isa(I)) continue; - if (!isa(I)) - return true; - for (const Use &U : I->operands()) { if (auto *UI = dyn_cast(U)) { if (!L.contains(UI)) @@ -148,8 +95,6 @@ containsUnreachableOrUnhandled(const Loop &L, Worklist.push_back(UI); continue; } - if (!isa(U)) - return true; } } return std::distance(Latch->begin(), Latch->end()) != Visited.size(); @@ -204,6 +149,60 @@ struct RecurrenceInfo { Instruction::BinaryOps BOWithConstOpToMatch = Instruction::BinaryOpsEnd); }; +/// Check the well-formedness of the (most|least) significant bit check given \p +/// ConditionalRecurrence, \p SimpleRecurrence, depending on \p +/// ByteOrderSwapped. We check that ConditionalRecurrence.Step is a +/// Select(Cmp()) where 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 that the LHS is (ConditionalRecurrence.Phi +/// [xor SimpleRecurrence.Phi]) in the big-endian case, and additionally check +/// for an AND with one in the little-endian case. 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, and this must be equal to ConditionalRecurrence.BO +/// (which is the bit-shift, as already checked by isBigEndianBitShift) for +/// well-formedness. +static bool +isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, + const RecurrenceInfo &SimpleRecurrence, + bool ByteOrderSwapped) { + auto *SI = cast(ConditionalRecurrence.Step); + DataLayout DL = SI->getParent()->getDataLayout(); + CmpPredicate Pred; + const Value *L; + const APInt *R; + Instruction *TV, *FV; + if (!match(SI, m_Select(m_ICmp(Pred, m_Value(L), m_APInt(R)), + m_Instruction(TV), m_Instruction(FV)))) + return false; + + // Match predicate with or without a SimpleRecurrence (the corresponding data + // is LHSAux). + auto MatchPred = + m_CombineOr(m_Specific(ConditionalRecurrence.Phi), + m_c_Xor(m_CastOrSelf(m_Specific(ConditionalRecurrence.Phi)), + m_CastOrSelf(m_Specific(SimpleRecurrence.Phi)))); + bool LWellFormed = ByteOrderSwapped ? match(L, MatchPred) + : match(L, m_c_And(MatchPred, m_One())); + if (!LWellFormed) + return false; + + KnownBits KnownR = KnownBits::makeConstant(*R); + unsigned BW = KnownR.getBitWidth(); + auto RCR = ConstantRange::fromKnownBits(KnownR, false); + auto AllowedByR = ConstantRange::makeAllowedICmpRegion(Pred, RCR); + ConstantRange CheckAllowedByR(APInt::getZero(BW), + ByteOrderSwapped ? APInt::getSignedMinValue(BW) + : APInt(BW, 1)); + + BinaryOperator *BitShift = ConditionalRecurrence.BO; + if (AllowedByR == CheckAllowedByR) + return TV == BitShift; + if (AllowedByR.inverse() == CheckAllowedByR) + return FV == BitShift; + return false; +} + /// Wraps llvm::matchSimpleRecurrence. Match a simple first order recurrence /// cycle of the form: /// @@ -375,7 +374,7 @@ CRCTable HashRecognize::genSarwateTable(const APInt &GenPoly, /// Checks that \p P1 and \p P2 are used together in an XOR in the use-def chain /// of \p SI's condition, ignoring any casts. The purpose of this function is to /// ensure that LHSAux from the SimpleRecurrence is used correctly in the CRC -/// computation. We cannot check the correctness of casts at this point. +/// computation. /// /// In other words, it checks for the following pattern: /// @@ -497,9 +496,8 @@ std::variant HashRecognize::recognizeCRC() const { "Expected ExtraConst in conditional recurrence"); const APInt &GenPoly = *ConditionalRecurrence.ExtraConst; - if (!isSignificantBitCheckWellFormed( - cast(ConditionalRecurrence.Step), - ConditionalRecurrence.BO, *ByteOrderSwapped)) + if (!isSignificantBitCheckWellFormed(ConditionalRecurrence, SimpleRecurrence, + *ByteOrderSwapped)) return "Malformed significant-bit check"; SmallVector Roots( @@ -508,8 +506,8 @@ std::variant HashRecognize::recognizeCRC() const { L.getLatchCmpInst(), Latch->getTerminator()}); if (SimpleRecurrence) Roots.push_back(SimpleRecurrence.BO); - if (containsUnreachableOrUnhandled(L, Roots)) - return "Found stray unvisited or unhandled instructions"; + if (containsUnreachable(L, Roots)) + return "Found stray unvisited instructions"; return PolynomialInfo(TC, LHS, GenPoly, ComputedValue, *ByteOrderSwapped, LHSAux); diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index 48464e69841f1..57898465b1d24 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll @@ -1109,7 +1109,7 @@ exit: ; preds = %loop define i16 @not.crc.unknown.value(i16 %msg, i16 %checksum, i16 %corrupt) { ; CHECK-LABEL: 'not.crc.unknown.value' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -1137,7 +1137,7 @@ exit: ; preds = %loop define i16 @not.crc.unknown.call.outside.loop(i16 %msg, i16 %checksum) { ; CHECK-LABEL: 'not.crc.unknown.call.outside.loop' ; CHECK-NEXT: Did not find a hash algorithm -; CHECK-NEXT: Reason: Found stray unvisited or unhandled instructions +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: %corrupt = call i16 @side.effect() @@ -1250,7 +1250,7 @@ exit: ; preds = %loop 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 or unhandled instructions +; CHECK-NEXT: Reason: Found stray unvisited instructions ; entry: br label %loop @@ -1276,7 +1276,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 or unhandled instructions +; CHECK-NEXT: Reason: Malformed significant-bit check ; entry: br label %loop @@ -1320,8 +1320,8 @@ exit: ; preds = %loop ret i16 %crc.next } -define i16 @not.crc.knownbits.sb.check.fail(i16 %crc.init) { -; CHECK-LABEL: 'not.crc.knownbits.sb.check.fail' +define i16 @not.crc.sb.check.patternmatch.fail(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.sb.check.patternmatch.fail' ; CHECK-NEXT: Did not find a hash algorithm ; CHECK-NEXT: Reason: Malformed significant-bit check ; @@ -1346,8 +1346,8 @@ exit: ; preds = %loop ret i16 %crc.next } -define i16 @not.crc.knownbits.sb.check.fail.call.outside.loop(i16 %crc.init) { -; CHECK-LABEL: 'not.crc.knownbits.sb.check.fail.call.outside.loop' +define i16 @not.crc.sb.check.patternmatch.fail.call.outside.loop(i16 %crc.init) { +; CHECK-LABEL: 'not.crc.sb.check.patternmatch.fail.call.outside.loop' ; CHECK-NEXT: Did not find a hash algorithm ; CHECK-NEXT: Reason: Malformed significant-bit check ; From 08d6c2dda75732786508d28e68f230102d28d15e Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Mon, 1 Sep 2025 12:31:53 +0100 Subject: [PATCH 4/6] [HashRecognize] Forbid sexts --- llvm/include/llvm/IR/PatternMatch.h | 8 ++ llvm/lib/Analysis/HashRecognize.cpp | 12 +-- .../HashRecognize/cyclic-redundancy-check.ll | 75 +++++++++++++++++++ 3 files changed, 89 insertions(+), 6 deletions(-) diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h index d68be03d4ba7d..2cb78904dd799 100644 --- a/llvm/include/llvm/IR/PatternMatch.h +++ b/llvm/include/llvm/IR/PatternMatch.h @@ -2283,6 +2283,14 @@ m_ZExtOrSExtOrSelf(const OpTy &Op) { return m_CombineOr(m_ZExtOrSExt(Op), Op); } +template +inline match_combine_or, + CastInst_match>, + OpTy> +m_ZExtOrTruncOrSelf(const OpTy &Op) { + return m_CombineOr(m_CombineOr(m_ZExt(Op), m_Trunc(Op)), Op); +} + template inline CastInst_match m_UIToFP(const OpTy &Op) { return CastInst_match(Op); diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index a8e480a78fde5..1a025a734b8ea 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -178,10 +178,10 @@ isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, // Match predicate with or without a SimpleRecurrence (the corresponding data // is LHSAux). - auto MatchPred = - m_CombineOr(m_Specific(ConditionalRecurrence.Phi), - m_c_Xor(m_CastOrSelf(m_Specific(ConditionalRecurrence.Phi)), - m_CastOrSelf(m_Specific(SimpleRecurrence.Phi)))); + auto MatchPred = m_CombineOr( + m_Specific(ConditionalRecurrence.Phi), + m_c_Xor(m_ZExtOrTruncOrSelf(m_Specific(ConditionalRecurrence.Phi)), + m_ZExtOrTruncOrSelf(m_Specific(SimpleRecurrence.Phi)))); bool LWellFormed = ByteOrderSwapped ? match(L, MatchPred) : match(L, m_c_And(MatchPred, m_One())); if (!LWellFormed) @@ -401,8 +401,8 @@ static bool isConditionalOnXorOfPHIs(const SelectInst *SI, const PHINode *P1, continue; // If we match an XOR of the two PHIs ignoring casts, we're done. - if (match(I, m_c_Xor(m_CastOrSelf(m_Specific(P1)), - m_CastOrSelf(m_Specific(P2))))) + if (match(I, m_c_Xor(m_ZExtOrTruncOrSelf(m_Specific(P1)), + m_ZExtOrTruncOrSelf(m_Specific(P2))))) return true; // Continue along the use-def chain. diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll index 57898465b1d24..432a4d72fafb4 100644 --- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll +++ b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll @@ -427,6 +427,53 @@ exit: ; preds = %loop ret i32 %crc.next } +define i16 @crc16.be.tc8.zext.data(i8 %msg, i16 %checksum) { +; CHECK-LABEL: 'crc16.be.tc8.zext.data' +; CHECK-NEXT: Found big-endian CRC-16 loop with trip count 8 +; CHECK-NEXT: Initial CRC: i16 %checksum +; CHECK-NEXT: Generating polynomial: 258 +; CHECK-NEXT: Computed CRC: %crc.next = select i1 %check.sb, i16 %crc.shl, i16 %crc.xor +; CHECK-NEXT: Auxiliary data: i8 %msg +; CHECK-NEXT: Computed CRC lookup table: +; CHECK-NEXT: 0 258 516 774 1032 1290 1548 1806 2064 2322 2580 2838 3096 3354 3612 3870 +; CHECK-NEXT: 4128 4386 4644 4902 5160 5418 5676 5934 6192 6450 6708 6966 7224 7482 7740 7998 +; CHECK-NEXT: 8256 8514 8772 9030 9288 9546 9804 10062 10320 10578 10836 11094 11352 11610 11868 12126 +; CHECK-NEXT: 12384 12642 12900 13158 13416 13674 13932 14190 14448 14706 14964 15222 15480 15738 15996 16254 +; CHECK-NEXT: 16512 16770 17028 17286 17544 17802 18060 18318 18576 18834 19092 19350 19608 19866 20124 20382 +; CHECK-NEXT: 20640 20898 21156 21414 21672 21930 22188 22446 22704 22962 23220 23478 23736 23994 24252 24510 +; CHECK-NEXT: 24768 25026 25284 25542 25800 26058 26316 26574 26832 27090 27348 27606 27864 28122 28380 28638 +; CHECK-NEXT: 28896 29154 29412 29670 29928 30186 30444 30702 30960 31218 31476 31734 31992 32250 32508 32766 +; CHECK-NEXT: 33024 32770 33540 33286 34056 33802 34572 34318 35088 34834 35604 35350 36120 35866 36636 36382 +; CHECK-NEXT: 37152 36898 37668 37414 38184 37930 38700 38446 39216 38962 39732 39478 40248 39994 40764 40510 +; CHECK-NEXT: 41280 41026 41796 41542 42312 42058 42828 42574 43344 43090 43860 43606 44376 44122 44892 44638 +; CHECK-NEXT: 45408 45154 45924 45670 46440 46186 46956 46702 47472 47218 47988 47734 48504 48250 49020 48766 +; CHECK-NEXT: 49536 49282 50052 49798 50568 50314 51084 50830 51600 51346 52116 51862 52632 52378 53148 52894 +; CHECK-NEXT: 53664 53410 54180 53926 54696 54442 55212 54958 55728 55474 56244 55990 56760 56506 57276 57022 +; CHECK-NEXT: 57792 57538 58308 58054 58824 58570 59340 59086 59856 59602 60372 60118 60888 60634 61404 61150 +; CHECK-NEXT: 61920 61666 62436 62182 62952 62698 63468 63214 63984 63730 64500 64246 65016 64762 65532 65278 +; +entry: + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i8 [ 0, %entry ], [ %iv.next, %loop ] + %data = phi i8 [ %msg, %entry ], [ %data.next, %loop ] + %crc = phi i16 [ %checksum, %entry ], [ %crc.next, %loop ] + %data.ext = zext i8 %data to i16 + %xor.crc.data = xor i16 %crc, %data.ext + %check.sb = icmp sge i16 %xor.crc.data, 0 + %crc.shl = shl i16 %crc, 1 + %crc.xor = xor i16 %crc.shl, 258 + %crc.next = select i1 %check.sb, i16 %crc.shl, i16 %crc.xor + %data.next = shl i8 %data, 1 + %iv.next = add nuw nsw i8 %iv, 1 + %exit.cond = icmp samesign ult i8 %iv, 7 + br i1 %exit.cond, label %loop, label %exit + +exit: ; preds = %loop + ret i16 %crc.next +} + ; Negative tests define i16 @not.crc.non.const.tc(i16 %crc.init, i32 %loop.limit) { @@ -1320,6 +1367,34 @@ exit: ; preds = %loop ret i16 %crc.next } +define i16 @not.crc.bad.cast.sext(i8 %msg, i16 %checksum) { +; CHECK-LABEL: 'not.crc.bad.cast.sext' +; CHECK-NEXT: Did not find a hash algorithm +; CHECK-NEXT: Reason: Recurrences not intertwined with XOR +; +entry: + br label %loop + +loop: ; preds = %loop, %entry + %iv = phi i8 [ 0, %entry ], [ %iv.next, %loop ] + %data = phi i8 [ %msg, %entry ], [ %data.next, %loop ] + %crc = phi i16 [ %checksum, %entry ], [ %crc.next, %loop ] + %data.ext = sext i8 %data to i16 + %xor.crc.data = xor i16 %crc, %data.ext + %check.sb = icmp sge i16 %xor.crc.data, 0 + %crc.shl = shl i16 %crc, 1 + %crc.xor = xor i16 %crc.shl, 258 + %crc.next = select i1 %check.sb, i16 %crc.shl, i16 %crc.xor + %data.next = shl i8 %data, 1 + %iv.next = add nuw nsw i8 %iv, 1 + %exit.cond = icmp samesign ult i8 %iv, 7 + br i1 %exit.cond, label %loop, label %exit + +exit: ; preds = %loop + ret i16 %crc.next +} + + define i16 @not.crc.sb.check.patternmatch.fail(i16 %crc.init) { ; CHECK-LABEL: 'not.crc.sb.check.patternmatch.fail' ; CHECK-NEXT: Did not find a hash algorithm From 0db408fc3c546cd5b2e0bed489fc75f9703f1f05 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Tue, 2 Sep 2025 18:11:51 +0100 Subject: [PATCH 5/6] [HashRecognize] Address review --- llvm/lib/Analysis/HashRecognize.cpp | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index 1a025a734b8ea..ad4c0ab413a91 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -73,8 +73,9 @@ using namespace SCEVPatternMatch; #define DEBUG_TYPE "hash-recognize" -/// Checks if Loop \p L contains instructions unreachable \p Roots on the -/// use-def chain. +/// Checks that all instructions reachable from \p Roots on the use-def chain +/// are contained within loop \p L, and that that are no stray instructions in +/// the loop not visited by the use-def walk. static bool containsUnreachable(const Loop &L, ArrayRef Roots) { SmallPtrSet Visited; @@ -93,7 +94,6 @@ static bool containsUnreachable(const Loop &L, if (!L.contains(UI)) return true; Worklist.push_back(UI); - continue; } } } @@ -157,17 +157,15 @@ struct RecurrenceInfo { /// compare are swapped). We check that the LHS is (ConditionalRecurrence.Phi /// [xor SimpleRecurrence.Phi]) in the big-endian case, and additionally check /// for an AND with one in the little-endian case. 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, and this must be equal to ConditionalRecurrence.BO -/// (which is the bit-shift, as already checked by isBigEndianBitShift) for -/// well-formedness. +/// against CheckAllowedByR, which is [0, smin) in the big-endian case, and +/// against [0, 1) in the little-endian case: CheckAllowedByR checks for +/// significant-bit-clear, and we match the corresponding arms of the select +/// against bit-shift and bit-shift-and-xor-gen-poly. static bool isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, const RecurrenceInfo &SimpleRecurrence, bool ByteOrderSwapped) { auto *SI = cast(ConditionalRecurrence.Step); - DataLayout DL = SI->getParent()->getDataLayout(); CmpPredicate Pred; const Value *L; const APInt *R; @@ -197,9 +195,11 @@ isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, BinaryOperator *BitShift = ConditionalRecurrence.BO; if (AllowedByR == CheckAllowedByR) - return TV == BitShift; + return TV == BitShift && + match(FV, m_c_Xor(m_Specific(BitShift), m_Constant())); if (AllowedByR.inverse() == CheckAllowedByR) - return FV == BitShift; + return FV == BitShift && + match(TV, m_c_Xor(m_Specific(BitShift), m_Constant())); return false; } @@ -219,8 +219,11 @@ isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, /// %BO = binop %step, %rec /// bool RecurrenceInfo::matchSimpleRecurrence(const PHINode *P) { - Phi = P; - return llvm::matchSimpleRecurrence(Phi, BO, Start, Step); + if (llvm::matchSimpleRecurrence(P, BO, Start, Step)) { + Phi = P; + return true; + } + return false; } /// Digs for a recurrence starting with \p V hitting the PHI node in a use-def From a695878bc1c9a6878cb17b94728a6fc9d96a08fc Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Sep 2025 14:31:02 +0100 Subject: [PATCH 6/6] [HashRecognize] Fix some nits --- llvm/lib/Analysis/HashRecognize.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp index ad4c0ab413a91..5b3448f5df35d 100644 --- a/llvm/lib/Analysis/HashRecognize.cpp +++ b/llvm/lib/Analysis/HashRecognize.cpp @@ -73,9 +73,9 @@ using namespace SCEVPatternMatch; #define DEBUG_TYPE "hash-recognize" -/// Checks that all instructions reachable from \p Roots on the use-def chain -/// are contained within loop \p L, and that that are no stray instructions in -/// the loop not visited by the use-def walk. +/// Checks if there's a stray instruction in the loop \p L outside of the +/// use-def chains from \p Roots, or if we escape the loop during the use-def +/// walk. static bool containsUnreachable(const Loop &L, ArrayRef Roots) { SmallPtrSet Visited; @@ -157,8 +157,8 @@ struct RecurrenceInfo { /// compare are swapped). We check that the LHS is (ConditionalRecurrence.Phi /// [xor SimpleRecurrence.Phi]) in the big-endian case, and additionally check /// for an AND with one in the little-endian case. We then check AllowedByR -/// against CheckAllowedByR, which is [0, smin) in the big-endian case, and -/// against [0, 1) in the little-endian case: CheckAllowedByR checks for +/// against CheckAllowedByR, which is [0, smin) in the big-endian case, and is +/// [0, 1) in the little-endian case. CheckAllowedByR checks for /// significant-bit-clear, and we match the corresponding arms of the select /// against bit-shift and bit-shift-and-xor-gen-poly. static bool @@ -196,10 +196,12 @@ isSignificantBitCheckWellFormed(const RecurrenceInfo &ConditionalRecurrence, BinaryOperator *BitShift = ConditionalRecurrence.BO; if (AllowedByR == CheckAllowedByR) return TV == BitShift && - match(FV, m_c_Xor(m_Specific(BitShift), m_Constant())); + match(FV, m_c_Xor(m_Specific(BitShift), + m_SpecificInt(*ConditionalRecurrence.ExtraConst))); if (AllowedByR.inverse() == CheckAllowedByR) return FV == BitShift && - match(TV, m_c_Xor(m_Specific(BitShift), m_Constant())); + match(TV, m_c_Xor(m_Specific(BitShift), + m_SpecificInt(*ConditionalRecurrence.ExtraConst))); return false; }