[SCEV] Add commutative add matchers (NFC)

[sivakusayan]

Add three new SCEV matchers for commutative addition, one for each wrapping flag that's valid on a SCEVAddExpr.

[github-actions]

🐧 Linux x64 Test Results

• 189428 tests passed
• 5036 tests skipped

✅ The build succeeded and all tests passed.

[sivakusayan]

My usecase is that I want to match load SCEVs that look like {{%ptr + offset}, +, step} or {{offset + %ptr}, +, step} and extract the offset from them. Please let me know if I'm missing something and there is already a way to do this.

Also, it seems like the failing build step is failing for other PRs too, here is one example. I would open an issue myself but I don't think I have permission to add the infrastructure label.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Sayan Sivakumaran (sivakusayan)

Changes

Add three new SCEV matchers for commutative addition, one for each wrapping flag that's valid on a SCEVAddExpr.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Analysis/ScalarEvolutionPatternMatch.h (+21)

diff --git a/llvm/include/llvm/Analysis/ScalarEvolutionPatternMatch.h b/llvm/include/llvm/Analysis/ScalarEvolutionPatternMatch.h
index f285eacc4c565..8dab5e0da1977 100644
--- a/llvm/include/llvm/Analysis/ScalarEvolutionPatternMatch.h
+++ b/llvm/include/llvm/Analysis/ScalarEvolutionPatternMatch.h
@@ -230,6 +230,27 @@ m_scev_Add(const Op0_t &Op0, const Op1_t &Op1) {
   return m_scev_Binary<SCEVAddExpr>(Op0, Op1);
 }
 
+template <typename Op0_t, typename Op1_t>
+inline SCEVBinaryExpr_match<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagAnyWrap, true>
+m_scev_c_Add(const Op0_t &Op0, const Op1_t &Op1) {
+  return m_scev_Binary<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagAnyWrap, true>(Op0,
+                                                                           Op1);
+}
+
+template <typename Op0_t, typename Op1_t>
+inline SCEVBinaryExpr_match<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagNUW, true>
+m_scev_c_NUWAdd(const Op0_t &Op0, const Op1_t &Op1) {
+  return m_scev_Binary<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagNUW, true>(Op0,
+                                                                       Op1);
+}
+
+template <typename Op0_t, typename Op1_t>
+inline SCEVBinaryExpr_match<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagNSW, true>
+m_scev_c_NSWAdd(const Op0_t &Op0, const Op1_t &Op1) {
+  return m_scev_Binary<SCEVAddExpr, Op0_t, Op1_t, SCEV::FlagNSW, true>(Op0,
+                                                                       Op1);
+}
+
 template <typename Op0_t, typename Op1_t>
 inline SCEVBinaryExpr_match<SCEVMulExpr, Op0_t, Op1_t>
 m_scev_Mul(const Op0_t &Op0, const Op1_t &Op1) {

[fhahn]

Is there any existing code that can make use of the new matchers? Would be good to have each one at least used once, to make sure they work correctly

[sivakusayan]

Is there any existing code that can make use of the new matchers? Would be good to have each one at least used once, to make sure they work correctly

I'll try to find some relevant places sometime this weekend or next week. I was planning on using them in a PR for this issue, but I figured I would commit this separately in case that PR had to be reverted for some reason.

[sivakusayan]

After looking at the ScalarEvolution class more closely, I'm noticing that there is a canonical ordering of operands in a SCEV expression by complexity. So a constant offset from a pointer should always be first in an expression, and I don't really need a commutative matcher for the cases {{%ptr + offset}, +, step} and {{offset + %ptr}, +, step} in my PR.

I locally changed m_scev_Add to always be commutative just to see what would happen, and I was surprised to see failing tests, so I guess there are subtle cases I'm not aware of where the ordering matters.

I'm going to convert this PR to a draft for now until I understand why those tests fail, but I think I ultimately won't need this PR. Sorry for taking up reviewer time.

[sivakusayan]

One example of a failing test from making add matchers commutative by default is llvm/test/Transforms/LoopVectorize/trip-count-expansion-may-introduce-ub.ll. This test fails because we have this line of code in SCEVURem_match:

const SCEV *A;
const SCEVMulExpr *Mul;
if (!SCEVPatternMatch::match(Expr, m_scev_Add(m_scev_Mul(Mul), m_SCEV(A))))
  return false;

Since sub-expressions in a SCEVAddExpr are ordered by complexity, this matcher only matched expressions where A had a higher complexity than a SCEVMulExpr. By making this commutative, it seems like unintended patterns were let through. I'm not sure why that caused failures though.

I'm very unfamiliar with much of the SCEV codebase, and this ended up being a rabbithole that I would prefer not to go down for now until I finish my other PRs, so I'll close this for now.