[SLP] Fix isCommutative to check uses of the original instruction instead of the converted instruction.

[HanKuanChen]

No description provided.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Han-Kuan Chen (HanKuanChen)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+12-6)
• (added) llvm/test/Transforms/SLPVectorizer/bbi-107513.ll (+34)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 6fcd606afaa22..3d24f31e37918 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -511,15 +511,15 @@ static bool isSplat(ArrayRef<Value *> VL) {
 }
 
 /// \returns True if \p I is commutative, handles CmpInst and BinaryOperator.
-static bool isCommutative(Instruction *I) {
+static bool isCommutative(Instruction *I, Instruction *InstWithUses) {
   if (auto *Cmp = dyn_cast<CmpInst>(I))
     return Cmp->isCommutative();
   if (auto *BO = dyn_cast<BinaryOperator>(I))
     return BO->isCommutative() ||
            (BO->getOpcode() == Instruction::Sub &&
-            !BO->hasNUsesOrMore(UsesLimit) &&
+            !InstWithUses->hasNUsesOrMore(UsesLimit) &&
             all_of(
-                BO->uses(),
+                InstWithUses->uses(),
                 [](const Use &U) {
                   // Commutative, if icmp eq/ne sub, 0
                   CmpPredicate Pred;
@@ -536,14 +536,16 @@ static bool isCommutative(Instruction *I) {
                           Flag->isOne());
                 })) ||
            (BO->getOpcode() == Instruction::FSub &&
-            !BO->hasNUsesOrMore(UsesLimit) &&
-            all_of(BO->uses(), [](const Use &U) {
+            !InstWithUses->hasNUsesOrMore(UsesLimit) &&
+            all_of(InstWithUses->uses(), [](const Use &U) {
               return match(U.getUser(),
                            m_Intrinsic<Intrinsic::fabs>(m_Specific(U.get())));
             }));
   return I->isCommutative();
 }
 
+static bool isCommutative(Instruction *I) { return isCommutative(I, I); }
+
 template <typename T>
 static std::optional<unsigned> getInsertExtractIndex(const Value *Inst,
                                                      unsigned Offset) {
@@ -2898,7 +2900,11 @@ class BoUpSLP {
           continue;
         }
         auto [SelectedOp, Ops] = convertTo(cast<Instruction>(V), S);
-        bool IsInverseOperation = !isCommutative(SelectedOp);
+        // We cannot check commutativity by the converted instruction
+        // (SelectedOp) because isCommutative also examines def-use
+        // relationships.
+        bool IsInverseOperation =
+            !isCommutative(SelectedOp, cast<Instruction>(V));
         for (unsigned OpIdx : seq<unsigned>(ArgSize)) {
           bool APO = (OpIdx == 0) ? false : IsInverseOperation;
           OpsVec[OpIdx][Lane] = {Operands[OpIdx][Lane], APO, false};
diff --git a/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll b/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll
new file mode 100644
index 0000000000000..14ead464943de
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll
@@ -0,0 +1,34 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=slp-vectorizer -S %s | FileCheck %s
+
+define i16 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND3:%.*]] = select i1 true, i16 1, i16 0
+; CHECK-NEXT:    ret i16 [[COND3]]
+;
+entry:
+  %sub = sub i16 0, -1
+  %cmp = icmp eq i16 %sub, 1
+
+  %sub1 = sub i16 0, -1
+  %cmp2 = icmp eq i16 %sub1, 1
+  %cond3 = select i1 %cmp2, i16 1, i16 0
+
+  %sub5 = sub nsw i16 0, 0
+  %cmp6 = icmp eq i16 %sub5, 0
+  %cmp9 = icmp eq i16 %sub5, 0
+
+  %sub12 = sub nsw i16 0, 0
+  %cmp13 = icmp eq i16 %sub12, 0
+
+  %sub16 = sub nsw i16 0, 0
+  %cmp17 = icmp eq i16 %sub16, 0
+
+  %sub20 = sub nsw i16 0, 0
+  %cmp21 = icmp eq i16 %sub20, 0
+  %cmp24 = icmp eq i16 %sub20, 0
+
+  ret i16 %cond3
+}
+

[alexey-bataev]

1. Add a comment for this function too
2. Do we need it at all?

[HanKuanChen]

We need it. isCommutative(Instruction *I) is used widely. Only interchangeable instruction analysis will change the parameter (InstWithUses).

[mikaelholmen]

I assume you want to give this regression test a more descriptive name than our downstream error report number :)

[mikaelholmen]

This patch seems to fix the problems I saw in #142347

I suggest mentioning the ticket number in the commit message.

[mikaelholmen]

When should this function be used, and when should it absolutely not be used?
I think that is what the comment should explain since it's far from obvious, and using it when it shouldn't be used may lead to miscompile (as seen in the issue I wrote).

[HanKuanChen]

There are already comments explaining the usage. In most cases, users should use the helper function.

[alexey-bataev]

I would add a note, at least, that this function should be used only if the original instruction is checked, not the representation. Or used the original isCommutative for all cases to avoid potential bugs

[mikaelholmen]

Ping. The miscompile is still there.

[alexey-bataev]

Add a reference to the tracking issue number