[HashRecognize] Rewrite arePHIsIntertwined

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 PHI
node arguments. Another issue is that arePHIsIntertwined is
unnecessarily complicated: the core functionality is to match a BinOp
that's a recurrence in both PHI nodes, ignoring zext and trunc casts.
Hence, rewrite it for clarity, and make it symmetric in both PHI
operands.

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.

Author: artagnon

llvm/include/llvm/IR/PatternMatch.h

@@ -2160,6 +2160,13 @@ m_ZExtOrSelf(const OpTy &Op) {
   return m_CombineOr(m_ZExt(Op), Op);
 }
 
+template <typename OpTy>
+inline match_combine_or<match_combine_or<CastInst_match<OpTy, ZExtInst>, OpTy>,
+                        CastInst_match<OpTy, TruncInst>>
+m_ZExtOrTruncOrSelf(const OpTy &Op) {
+  return m_CombineOr(m_ZExtOrSelf(Op), m_Trunc(Op));
+}
+
 template <typename OpTy>
 inline match_combine_or<CastInst_match<OpTy, SExtInst>, OpTy>
 m_SExtOrSelf(const OpTy &Op) {

llvm/lib/Analysis/HashRecognize.cpp

@@ -497,45 +497,37 @@ 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.
+/// Checks that \p Needle and \p Reference are used together in a
+/// BinaryOperator, which is a recurrence over both, ignoring zext and trunc.
+/// Additionally checks that the BinaryOperator has opcode \p BOToMatch, which
+/// is XOR in the case of CRC. In other words, it checks for the following
+/// pattern:
+///
+/// loop:
+///   %needle = phi [_, %entry], [%needle.next, %loop]
+///   %reference = phi [_, %entry], [%reference.next, %loop]
+///   ...
+///   _ = BOTOMatch ((trunc|zext|self) %needle) ((trunc|zext|self) %reference)
+///
 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.
-  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;
-  }
-
-  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).
-    if (isa<PHINode>(I))
-      return I == Reference;
-
-    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))
-          Worklist.push_back(UI);
-  }
-  return false;
+  return any_of(ArrayRef({Needle, Reference}), [&](const PHINode *S) {
+    return count_if(S->users(), [&](const User *U) {
+             auto *BO = dyn_cast<BinaryOperator>(U);
+             if (!BO)
+               return false;
+             if (BOToMatch != Instruction::BinaryOpsEnd &&
+                 BO->getOpcode() != BOToMatch)
+               return false;
+             return all_of(BO->operands(), [Needle, Reference](const Use &U) {
+               return match(
+                   U.get(),
+                   m_CombineOr(m_ZExtOrTruncOrSelf(m_Specific(Reference)),
+                               m_ZExtOrTruncOrSelf(m_Specific(Needle))));
+             });
+           }) == 1;
+  });
 }
 
 // Recognizes a multiplication or division by the constant two, using SCEV. By
@@ -588,7 +580,7 @@ HashRecognize::recognizeCRC() const {
       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";
+      return "Recurrences not intertwined with XOR";
   }
 
   // Make sure that the computed value is used in the exit block: this should be

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

@@ -147,7 +147,7 @@ exit:                                              ; preds = %loop
 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: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT:  Reason: Loop iterations exceed bitwidth of result
 ;
 entry:
   br label %loop
@@ -629,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
@@ -868,7 +868,7 @@ exit:                                              ; preds = %loop
 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: Simple recurrence doesn't use conditional recurrence with XOR
+; CHECK-NEXT:  Reason: Expected bottom 8 bits zero (????????00001011)
 ;
 entry:
   br label %loop
@@ -895,7 +895,7 @@ exit:                                              ; preds = %loop
 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
@@ -923,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 not intertwined with XOR
 ;
 entry:
   br label %loop
@@ -952,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