[IR] Remove non-canonical matchings

[AZero13]

No description provided.

[AZero13]

@nikic This is the follow up to the NFC one.

[llvmbot]

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

---

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

5 Files Affected:

• (modified) llvm/include/llvm/IR/PatternMatch.h (+2-2)
• (modified) llvm/lib/Analysis/ValueTracking.cpp (+1-1)
• (modified) llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp (+1-1)
• (modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+1-1)
• (modified) llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp (+2-4)

diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
index 526b7258b8ab7..72ad2800f5dee 100644
--- a/llvm/include/llvm/IR/PatternMatch.h
+++ b/llvm/include/llvm/IR/PatternMatch.h
@@ -2665,14 +2665,14 @@ m_NSWNeg(const ValTy &V) {
 template <typename ValTy>
 inline BinaryOp_match<cst_pred_ty<is_all_ones>, ValTy, Instruction::Xor, true>
 m_Not(const ValTy &V) {
-  return m_c_Xor(m_AllOnes(), V);
+  return m_Xor(V, m_AllOnes());
 }
 
 template <typename ValTy>
 inline BinaryOp_match<cst_pred_ty<is_all_ones, false>, ValTy, Instruction::Xor,
                       true>
 m_NotForbidPoison(const ValTy &V) {
-  return m_c_Xor(m_AllOnesForbidPoison(), V);
+  return m_Xor(V, m_AllOnesForbidPoison());
 }
 
 /// Matches an SMin with LHS and RHS in either order.
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 1dfc6cfac4551..1516885036173 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -924,7 +924,7 @@ getKnownBitsFromAndXorOr(const Operator *I, const APInt &DemandedElts,
     // Demanded) == (xor(x, x-1) & Demanded). Extend the xor pattern
     // to use arbitrary C if xor(x, x-C) as the same as xor(x, x-1).
     if (HasKnownOne &&
-        match(I, m_c_Xor(m_Value(X), m_c_Add(m_Deferred(X), m_AllOnes())))) {
+        match(I, m_c_Xor(m_Value(X), m_Add(m_Deferred(X), m_AllOnes())))) {
       const KnownBits &XBits = I->getOperand(0) == X ? KnownLHS : KnownRHS;
       KnownOut = XBits.blsmsk();
     }
diff --git a/llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp b/llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp
index 5a383b23a8338..1f3f2f0427912 100644
--- a/llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp
+++ b/llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp
@@ -829,7 +829,7 @@ bool PolynomialMultiplyRecognize::matchRightShift(SelectInst *SelI,
     return false;
 
   Value *X = nullptr;
-  if (!match(C, m_c_And(m_Value(X), m_One())))
+  if (!match(C, m_And(m_Value(X), m_One())))
     return false;
   // Matched: select (X & 1) == +++ ? ... : ...
   //          select (X & 1) != +++ ? ... : ...
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index b9148999ff395..f917e93493950 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -30557,7 +30557,7 @@ static std::pair<Value *, BitTestKind> FindSingleBitChange(Value *V) {
     bool Not = false;
     // Check if we have a NOT
     Value *PeekI;
-    if (match(I, m_c_Xor(m_Value(PeekI), m_AllOnes())) ||
+    if (match(I, m_Not(m_Value(PeekI))) ||
         match(I, m_Sub(m_AllOnes(), m_Value(PeekI)))) {
       Not = true;
       I = dyn_cast<Instruction>(PeekI);
diff --git a/llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
index 75910d7b698aa..ae410bd9f3ed6 100644
--- a/llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
@@ -425,14 +425,12 @@ void StraightLineStrengthReduce::allocateCandidatesAndFindBasisForAdd(
 
 // Returns true if A matches B + C where C is constant.
 static bool matchesAdd(Value *A, Value *&B, ConstantInt *&C) {
-  return (match(A, m_Add(m_Value(B), m_ConstantInt(C))) ||
-          match(A, m_Add(m_ConstantInt(C), m_Value(B))));
+  return match(A, m_Add(m_Value(B), m_ConstantInt(C)));
 }
 
 // Returns true if A matches B | C where C is constant.
 static bool matchesOr(Value *A, Value *&B, ConstantInt *&C) {
-  return (match(A, m_Or(m_Value(B), m_ConstantInt(C))) ||
-          match(A, m_Or(m_ConstantInt(C), m_Value(B))));
+  return match(A, m_Or(m_Value(B), m_ConstantInt(C)));
 }
 
 void StraightLineStrengthReduce::allocateCandidatesAndFindBasisForMul(

[nikic]

This is not NFC, and there are test failures.

[AZero13]

This is not NFC, and there are test failures.

Not anymore

[AZero13]

@nikic Is this good now?

[RKSimon]

What are you trying to accomplish with this patch? The SLSR change looks sensible, we use the m_c_Add/m_c_Or matchers to avoid duplication in commutative ops. But what are you trying to avoid with the other changes? Are you certain that we won't see constants on the LHS on the ADD/AND ops? Canonicalization isn't always done in time - especially in general code like getKnownBitsFromAndXorOr.

[nikic]

LGTM from my side. Non-canonical patterns should only be handled if there is a specific need, in which case there should at minimum be a test case that justifies it, which is not present here.

[goldsteinn]

Do we need commutative version here/above?

[AZero13]

Yeah. Otherwise tests break.

[goldsteinn]

LGTM