[llvm] [SLP] Fix misvectorization in commutative to non-commutative conversion (PR #185230)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: tudinhh (tudinhh)

<details>
<summary>Changes</summary>

**Summary**
Fixes a miscompilation where commutative operations (e.g., or, and, mul) with a left-hand side constant were incorrectly transformed into non-commutative operations (e.g., shl, sub).

**The Problem**
In BinOpSameOpcodeHelper::getOperand, when a constant is at Pos == 0, the helper was failing to swap operand order for new non-commutative target opcodes. This resulted in inverted logic, such as transforming `or 0, %x` into `shl 0, %x` (resulting in 0) instead of the correct `%x << 0`.

**The Fix**
The existing logic only protected the Sub opcode. This patch generalizes the fix to all non-commutative instructions by using !Instruction::isCommutative(ToOpcode). This ensures that for any directional operation, the variable is correctly placed on the LHS and the constant on the RHS.

**Changes**
SLPVectorizer.cpp: Replaced the specific Sub check with a general isCommutative check.

Regression Test: Added lhs-constant-non-cummutative.ll to cover shl, sub, and ashr targets.

Fixes #<!-- -->185186

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


2 Files Affected:

- (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+4-6) 
- (added) llvm/test/Transforms/SLPVectorizer/X86/lhs-constant-non-cummutative.ll (+39) 


``````````diff
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 631af61e2cfba..df7885e31741e 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -1132,13 +1132,11 @@ class BinOpSameOpcodeHelper {
         break;
       }
       Value *LHS = I->getOperand(1 - Pos);
-      // constant + x cannot be -constant - x
-      // instead, it should be x - -constant
-      if (Pos == 1 ||
-          ((FromOpcode == Instruction::Add || FromOpcode == Instruction::Or ||
-            FromOpcode == Instruction::Xor) &&
-           ToOpcode == Instruction::Sub))
+      // If the target opcode is non-commutative (e.g., shl, sub),
+      // force the variable to the left and the constant to the right.
+      if (Pos == 1 || !Instruction::isCommutative(ToOpcode))
         return SmallVector<Value *>({LHS, RHS});
+
       return SmallVector<Value *>({RHS, LHS});
     }
   };
diff --git a/llvm/test/Transforms/SLPVectorizer/X86/lhs-constant-non-cummutative.ll b/llvm/test/Transforms/SLPVectorizer/X86/lhs-constant-non-cummutative.ll
new file mode 100644
index 0000000000000..5a33d5f5bac5b
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/X86/lhs-constant-non-cummutative.ll
@@ -0,0 +1,39 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S --passes=slp-vectorizer -slp-threshold=-50 -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
+
+define void @or_lhs_to_shl(ptr %p, ptr %s) {
+; CHECK-LABEL: define void @or_lhs_to_shl(
+; CHECK-SAME: ptr [[P:%.*]], ptr [[S:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[TMP0:%.*]] = load <4 x i16>, ptr [[P]], align 2
+; CHECK-NEXT:    [[TMP1:%.*]] = shl <4 x i16> [[TMP0]], <i16 1, i16 2, i16 3, i16 0>
+; CHECK-NEXT:    store <4 x i16> [[TMP1]], ptr [[S]], align 2
+; CHECK-NEXT:    ret void
+;
+entry:
+  %p1 = getelementptr i16, ptr %p, i64 1
+  %p2 = getelementptr i16, ptr %p, i64 2
+  %p3 = getelementptr i16, ptr %p, i64 3
+
+  %l0 = load i16, ptr %p, align 2
+  %l1 = load i16, ptr %p1, align 2
+  %l2 = load i16, ptr %p2, align 2
+  %l3 = load i16, ptr %p3, align 2
+
+  ; 3 shl instructions force the vectorizer to choose shl as the target
+  %op0 = shl i16 %l0, 1
+  %op1 = shl i16 %l1, 2
+  %op2 = shl i16 %l2, 3
+  %op3 = or i16 0, %l3 ; The buggy instruction
+
+  %s1 = getelementptr i16, ptr %s, i64 1
+  %s2 = getelementptr i16, ptr %s, i64 2
+  %s3 = getelementptr i16, ptr %s, i64 3
+
+  store i16 %op0, ptr %s, align 2
+  store i16 %op1, ptr %s1, align 2
+  store i16 %op2, ptr %s2, align 2
+  store i16 %op3, ptr %s3, align 2
+
+  ret void
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/185230