[ValueTracking] Remove opcode whitelist from matchSimpleRecurrence.

[rj-jesus]

Note: I'm not sure how to test this, but #143878 is affected by this change.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Ricardo Jesus (rj-jesus)

Changes

Note: I'm not sure how to test this, but #143878 is affected by this change.

---

Full diff: https://github.com/llvm/llvm-project/pull/144031.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+3-1)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 0a460786d00ea..a9edb02440b5e 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -9072,7 +9072,7 @@ bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
     switch (Opcode) {
     default:
       continue;
-    // TODO: Expand list -- xor, gep, uadd.sat etc.
+    // TODO: Expand list -- gep, uadd.sat etc.
     case Instruction::LShr:
     case Instruction::AShr:
     case Instruction::Shl:
@@ -9082,7 +9082,9 @@ bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
     case Instruction::URem:
     case Instruction::And:
     case Instruction::Or:
+    case Instruction::Xor:
     case Instruction::Mul:
+    case Instruction::FAdd:
     case Instruction::FMul: {
       Value *LL = LU->getOperand(0);
       Value *LR = LU->getOperand(1);

[nikic]

Should we just remove the opcode white list entirely? Ultimately the caller has to decide which opcodes it supports, so I'm not sure this list inside matchSimpleRecurrence really has value.

[rj-jesus]

Thanks, that makes sense to me. Removing the whitelist leads to an ICE on:

LLVM :: Analysis/HashRecognize/cyclic-redundancy-check.ll

Presumably due to an unhandled opcode in llvm/lib/Analysis/HashRecognize.cpp (FDIV it seems).

This seems to sort it:

diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp
index b245548dea6d..cf172f6871cf 100644
--- a/llvm/lib/Analysis/HashRecognize.cpp
+++ b/llvm/lib/Analysis/HashRecognize.cpp
@@ -580,6 +580,8 @@ HashRecognize::recognizeCRC() const {
   if (!ByteOrderSwapped)
     return "Loop with non-unit bitshifts";
   if (SimpleRecurrence) {
+    if (!SE.isSCEVable(SimpleRecurrence.BO->getType()))
+      return "Type is not SCEVable";
     if (isBigEndianBitShift(SE.getSCEV(SimpleRecurrence.BO)) !=
         ByteOrderSwapped)
       return "Loop with non-unit bitshifts";

Does that look like a reasonable change?
CC @artagnon

[nikic]

@rj-jesus I'd check for integer specifically instead of isSCEVable. I'd also move the getSCEV call into isBigEndianBitShift to cover both call-sites. (Generally the use of SCEV for that check seems a bit questionable, but that's unrelated to this patch).

[rj-jesus]

@nikic Thanks very much, I've made the changes you suggested, including removing the whitelist from matchSimpleRecurrence. I've also added a new test to llvm/test/Analysis/HashRecognize/cyclic-redundancy-check.ll since with the new SCEV changes we no longer exercise the code path that the previously failing test covered.

Please let me know if you have any other suggestions or comments. :)

[nikic]

LGTM

[artagnon]

LGTM, thanks!