[llvm] cbfd0d6 - [HashRecognize] Rewrite arePHIsIntertwined (#144878)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 2 07:36:31 PDT 2025
Author: Ramkumar Ramachandra
Date: 2025-07-02T15:36:27+01:00
New Revision: cbfd0d68ea5084704bd9ff0c6c51e04fa3fff4be
URL: https://github.com/llvm/llvm-project/commit/cbfd0d68ea5084704bd9ff0c6c51e04fa3fff4be
DIFF: https://github.com/llvm/llvm-project/commit/cbfd0d68ea5084704bd9ff0c6c51e04fa3fff4be.diff
LOG: [HashRecognize] Rewrite arePHIsIntertwined (#144878)
The test crc8.le.tc16 is a valid CRC algorithm, but isn't recognized as
such due to a buggy arePHIsIntertwined, which is asymmetric in its
PHINode arguments. There is also a fundamental correctness issue: the
core functionality is to match a XOR that's a recurrence in both PHI
nodes, ignoring casts, but the user of the XOR is never checked. Rewrite
and rename the function.
crc8.le.tc16 is still not recognized as a valid CRC algorithm, due to an
incorrect check for loop iterations exceeding the bitwidth of the
result: in reality, it should not exceed the bitwidth of LHSAux, but we
leave this fix to a follow-up.
Co-authored-by: Piotr Fusik <p.fusik at samsung.com>
Added:
Modified:
llvm/include/llvm/IR/PatternMatch.h
llvm/lib/Analysis/HashRecognize.cpp
llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
index 1f86cdfd94e17..ed9b83d5d4361 100644
--- a/llvm/include/llvm/IR/PatternMatch.h
+++ b/llvm/include/llvm/IR/PatternMatch.h
@@ -2119,6 +2119,13 @@ m_IntToPtr(const OpTy &Op) {
return CastOperator_match<OpTy, Instruction::IntToPtr>(Op);
}
+/// Matches any cast or self. Used to ignore casts.
+template <typename OpTy>
+inline match_combine_or<CastInst_match<OpTy, CastInst>, OpTy>
+m_CastOrSelf(const OpTy &Op) {
+ return m_CombineOr(CastInst_match<OpTy, CastInst>(Op), Op);
+}
+
/// Matches Trunc.
template <typename OpTy>
inline CastInst_match<OpTy, TruncInst> m_Trunc(const OpTy &Op) {
diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp
index 1a5dfbe4bed6e..9e19fca67e7ad 100644
--- a/llvm/lib/Analysis/HashRecognize.cpp
+++ b/llvm/lib/Analysis/HashRecognize.cpp
@@ -497,42 +497,46 @@ CRCTable HashRecognize::genSarwateTable(const APInt &GenPoly,
return Table;
}
-/// Checks if \p Reference is reachable from \p Needle on the use-def chain, and
-/// that there are no stray PHI nodes while digging the use-def chain. \p
-/// BOToMatch is a CRC peculiarity: at least one of the Users of Needle needs to
-/// match this OpCode, which is XOR for CRC.
-static bool arePHIsIntertwined(
- const PHINode *Needle, const PHINode *Reference, const Loop &L,
- Instruction::BinaryOps BOToMatch = Instruction::BinaryOpsEnd) {
- // Initialize the worklist with Users of the Needle.
+/// 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.
+///
+/// In other words, it checks for the following pattern:
+///
+/// loop:
+/// %P1 = phi [_, %entry], [%P1.next, %loop]
+/// %P2 = phi [_, %entry], [%P2.next, %loop]
+/// ...
+/// %xor = xor (CastOrSelf %P1), (CastOrSelf %P2)
+///
+/// where %xor is in the use-def chain of \p SI's condition.
+static bool isConditionalOnXorOfPHIs(const SelectInst *SI, const PHINode *P1,
+ const PHINode *P2, const Loop &L) {
SmallVector<const Instruction *> Worklist;
- for (const User *U : Needle->users()) {
- if (auto *UI = dyn_cast<Instruction>(U))
- if (L.contains(UI))
- Worklist.push_back(UI);
- }
- // BOToMatch is usually XOR for CRC.
- if (BOToMatch != Instruction::BinaryOpsEnd) {
- if (count_if(Worklist, [BOToMatch](const Instruction *I) {
- return I->getOpcode() == BOToMatch;
- }) != 1)
- return false;
- }
+ // matchConditionalRecurrence has already ensured that the SelectInst's
+ // condition is an Instruction.
+ Worklist.push_back(cast<Instruction>(SI->getCondition()));
while (!Worklist.empty()) {
const Instruction *I = Worklist.pop_back_val();
- // Since Needle is never pushed onto the Worklist, I must either be the
- // Reference PHI node (in which case we're done), or a stray PHI node (in
- // which case we abort).
+ // Don't add a PHI's operands to the Worklist.
if (isa<PHINode>(I))
- return I == Reference;
+ 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)))))
+ return true;
+ // Continue along the use-def chain.
for (const Use &U : I->operands())
if (auto *UI = dyn_cast<Instruction>(U))
- // Don't push Needle back onto the Worklist.
- if (UI != Needle && L.contains(UI))
+ if (L.contains(UI))
Worklist.push_back(UI);
}
return false;
@@ -586,9 +590,19 @@ HashRecognize::recognizeCRC() const {
if (SimpleRecurrence) {
if (isBigEndianBitShift(SimpleRecurrence.BO, SE) != ByteOrderSwapped)
return "Loop with non-unit bitshifts";
- if (!arePHIsIntertwined(SimpleRecurrence.Phi, ConditionalRecurrence.Phi, L,
- Instruction::BinaryOps::Xor))
- return "Simple recurrence doesn't use conditional recurrence with XOR";
+
+ // Ensure that the PHIs have exactly two uses:
+ // the bit-shift, and the XOR (or a cast feeding into the XOR).
+ if (!ConditionalRecurrence.Phi->hasNUses(2) ||
+ !SimpleRecurrence.Phi->hasNUses(2))
+ return "Recurrences have stray uses";
+
+ // Check that the SelectInst ConditionalRecurrence.Step is conditional on
+ // the XOR of SimpleRecurrence.Phi and ConditionalRecurrence.Phi.
+ if (!isConditionalOnXorOfPHIs(cast<SelectInst>(ConditionalRecurrence.Step),
+ SimpleRecurrence.Phi,
+ ConditionalRecurrence.Phi, L))
+ return "Recurrences not intertwined with XOR";
}
// Make sure that the computed value is used in the exit block: this should be
diff --git a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll
index 0366684a13b54..88fd8bf3c2487 100644
--- a/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll
+++ b/llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll
@@ -144,6 +144,34 @@ exit: ; preds = %loop
ret i16 %crc.next
}
+define i8 @crc8.le.tc16(i16 %msg, i8 %checksum) {
+; CHECK-LABEL: 'crc8.le.tc16'
+; CHECK-NEXT: Did not find a hash algorithm
+; CHECK-NEXT: Reason: Loop iterations exceed bitwidth of result
+;
+entry:
+ br label %loop
+
+loop: ; preds = %loop, %entry
+ %iv = phi i8 [ 0, %entry ], [ %iv.next, %loop ]
+ %crc = phi i8 [ %checksum, %entry ], [ %crc.next, %loop ]
+ %data = phi i16 [ %msg, %entry ], [ %data.next, %loop ]
+ %data.trunc = trunc i16 %data to i8
+ %xor.crc.data = xor i8 %crc, %data.trunc
+ %and.crc.data = and i8 %xor.crc.data, 1
+ %data.next = lshr i16 %data, 1
+ %check.sb = icmp eq i8 %and.crc.data, 0
+ %crc.lshr = lshr i8 %crc, 1
+ %crc.xor = xor i8 %crc.lshr, 29
+ %crc.next = select i1 %check.sb, i8 %crc.lshr, i8 %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 i8 %crc.next
+}
+
define i16 @crc16.be.tc8.crc.init.li(i16 %checksum, i8 %msg) {
; CHECK-LABEL: 'crc16.be.tc8.crc.init.li'
; CHECK-NEXT: Found big-endian CRC-16 loop with trip count 8
@@ -601,7 +629,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: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT: Reason: Bad RHS of significant-bit-check
;
entry:
br label %loop
@@ -610,9 +638,8 @@ 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 ]
- %crc.lshr = lshr i16 %crc, 8
%data.ext = zext i8 %data to i16
- %xor.crc.data = xor i16 %crc.lshr, %data.ext
+ %xor.crc.data = xor i16 %crc, %data.ext
%check.sb = icmp samesign ult i16 %xor.crc.data, 128
%crc.shl = shl i16 %crc, 1
%crc.xor = xor i16 %crc.shl, 258
@@ -838,10 +865,37 @@ exit: ; preds = %loop
ret i16 %crc.next
}
+define i16 @not.crc.bad.cast(i8 %msg, i16 %checksum) {
+; CHECK-LABEL: 'not.crc.bad.cast'
+; CHECK-NEXT: Did not find a hash algorithm
+; CHECK-NEXT: Reason: Expected bottom 8 bits zero (????????00001011)
+;
+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 slt i16 %xor.crc.data, 0
+ %crc.shl = shl i16 %crc, 1
+ %crc.xor = xor i16 %crc.shl, 29
+ %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 i32 @not.crc.dead.msg.bad.use(i32 %checksum, i32 %msg) {
; CHECK-LABEL: 'not.crc.dead.msg.bad.use'
; CHECK-NEXT: Did not find a hash algorithm
-; CHECK-NEXT: Reason: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT: Reason: Recurrences not intertwined with XOR
;
entry:
br label %loop
@@ -869,7 +923,7 @@ exit: ; preds = %loop
define i16 @not.crc.dead.msg.no.use(i8 %msg, i16 %checksum) {
; CHECK-LABEL: 'not.crc.dead.msg.no.use'
; CHECK-NEXT: Did not find a hash algorithm
-; CHECK-NEXT: Reason: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT: Reason: Recurrences have stray uses
;
entry:
br label %loop
@@ -898,7 +952,7 @@ exit: ; preds = %loop
define i32 @not.crc.dead.msg.wrong.op(i32 %checksum, i32 %msg) {
; CHECK-LABEL: 'not.crc.dead.msg.wrong.op'
; CHECK-NEXT: Did not find a hash algorithm
-; CHECK-NEXT: Reason: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT: Reason: Recurrences not intertwined with XOR
;
entry:
br label %loop
@@ -922,6 +976,120 @@ exit: ; preds = %loop
ret i32 %crc.next
}
+define i16 @not.crc.dead.msg.xor.notin.select.chain(i16 %msg, i16 %checksum) {
+; CHECK-LABEL: 'not.crc.dead.msg.xor.notin.select.chain'
+; CHECK-NEXT: Did not find a hash algorithm
+; CHECK-NEXT: Reason: Recurrences have stray uses
+;
+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
+ %or.crc.data = or i16 %crc, %data
+ %and.crc.data = and i16 %or.crc.data, 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.bad.xor.crc.data(i16 %msg, i16 %checksum) {
+; CHECK-LABEL: 'not.crc.bad.xor.crc.data'
+; CHECK-NEXT: Did not find a hash algorithm
+; CHECK-NEXT: Reason: Recurrences have stray uses
+;
+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
+ %mul.corrupt = mul i16 %xor.crc.data, 0
+ %xor.crc.data.corrupt = xor i16 %mul.corrupt, %crc
+ %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.dead.msg.or.zero(i16 %msg, i16 %checksum) {
+; CHECK-LABEL: 'not.crc.dead.msg.or.zero'
+; CHECK-NEXT: Did not find a hash algorithm
+; CHECK-NEXT: Reason: Recurrences have stray uses
+;
+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
+ %mul.corrupt = mul i16 %xor.crc.data, 0
+ %or.crc.data.corrupt = or i16 %mul.corrupt, %crc
+ %and.crc.data = and i16 %or.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.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
+;
+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, %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.float.simple.recurrence(float %msg, i16 %checksum) {
; CHECK-LABEL: 'not.crc.float.simple.recurrence'
; CHECK-NEXT: Did not find a hash algorithm
More information about the llvm-commits
mailing list