From 5365abc1d6a89e3a3e5b9fbb18029ff306256c2d Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Fri, 6 Jun 2025 01:33:05 -0700 Subject: [PATCH 1/6] [SLP] Pre-commit test. --- .../Transforms/SLPVectorizer/bbi-107513.ll | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 llvm/test/Transforms/SLPVectorizer/bbi-107513.ll diff --git a/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll b/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll new file mode 100644 index 0000000000000..4dd768e704c75 --- /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 false, 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 +} + From 0b666c769c38a9e9ca64f2b127a037a1817b043b Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Fri, 6 Jun 2025 01:43:19 -0700 Subject: [PATCH 2/6] [SLP] Fix isCommutative to check uses of the original instruction instead of the converted instruction. --- .../lib/Transforms/Vectorize/SLPVectorizer.cpp | 18 ++++++++++++------ .../Transforms/SLPVectorizer/bbi-107513.ll | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) 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 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(I)) return Cmp->isCommutative(); if (auto *BO = dyn_cast(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(m_Specific(U.get()))); })); return I->isCommutative(); } +static bool isCommutative(Instruction *I) { return isCommutative(I, I); } + template static std::optional getInsertExtractIndex(const Value *Inst, unsigned Offset) { @@ -2898,7 +2900,11 @@ class BoUpSLP { continue; } auto [SelectedOp, Ops] = convertTo(cast(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(V)); for (unsigned OpIdx : seq(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 index 4dd768e704c75..14ead464943de 100644 --- a/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll +++ b/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll @@ -4,7 +4,7 @@ define i16 @foo() { ; CHECK-LABEL: @foo( ; CHECK-NEXT: entry: -; CHECK-NEXT: [[COND3:%.*]] = select i1 false, i16 1, i16 0 +; CHECK-NEXT: [[COND3:%.*]] = select i1 true, i16 1, i16 0 ; CHECK-NEXT: ret i16 [[COND3]] ; entry: From ed476d6bb8936392851f1e1ee450663576457e6e Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Sat, 7 Jun 2025 02:14:51 -0700 Subject: [PATCH 3/6] add comments --- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp index 3d24f31e37918..55f8719c24970 100644 --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp @@ -511,6 +511,12 @@ static bool isSplat(ArrayRef VL) { } /// \returns True if \p I is commutative, handles CmpInst and BinaryOperator. +/// For BinaryOperator, it also checks if \p InstWithUses is used in specific +/// patterns that make it effectively commutative (like equality comparisons +/// with zero). +/// \param I The instruction to check for commutativity +/// \param InstWithUses The instruction whose uses are analyzed for special +/// patterns static bool isCommutative(Instruction *I, Instruction *InstWithUses) { if (auto *Cmp = dyn_cast(I)) return Cmp->isCommutative(); From 9655dad2040c3ac7735660b5eff40516ad34775c Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Sat, 7 Jun 2025 05:40:06 -0700 Subject: [PATCH 4/6] add comments --- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp index 55f8719c24970..67c0eeb4b7033 100644 --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp @@ -514,6 +514,10 @@ static bool isSplat(ArrayRef VL) { /// For BinaryOperator, it also checks if \p InstWithUses is used in specific /// patterns that make it effectively commutative (like equality comparisons /// with zero). +/// In most cases, users should not call this function directly (since \p I and +/// \p InstWithUses are the same). However, when analyzing interchangeable +/// instructions, we need to use the converted opcode along with the original +/// uses. /// \param I The instruction to check for commutativity /// \param InstWithUses The instruction whose uses are analyzed for special /// patterns @@ -550,6 +554,7 @@ static bool isCommutative(Instruction *I, Instruction *InstWithUses) { return I->isCommutative(); } +/// This is a helper function to check whether \p I is commutative. static bool isCommutative(Instruction *I) { return isCommutative(I, I); } template From 4867c935664ea106e15ecce3425dd03e8886ed22 Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Mon, 16 Jun 2025 20:42:27 -0700 Subject: [PATCH 5/6] add comment --- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp index 67c0eeb4b7033..b332da0eb1a49 100644 --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp @@ -555,6 +555,13 @@ static bool isCommutative(Instruction *I, Instruction *InstWithUses) { } /// This is a helper function to check whether \p I is commutative. +/// This is a convenience wrapper that calls the two-parameter version of +/// isCommutative with the same instruction for both parameters. This is +/// the common case where the instruction being checked for commutativity +/// is the same as the instruction whose uses are analyzed for special +/// patterns (see the two-parameter version above for details). +/// \param I The instruction to check for commutativity +/// \returns true if the instruction is commutative, false otherwise static bool isCommutative(Instruction *I) { return isCommutative(I, I); } template From 149b928a18a063021b83022c7ceeaf5fe3e03a64 Mon Sep 17 00:00:00 2001 From: Han-Kuan Chen Date: Tue, 17 Jun 2025 06:40:47 -0700 Subject: [PATCH 6/6] rename file and better function name --- .../SLPVectorizer/{bbi-107513.ll => isCommutative.ll} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename llvm/test/Transforms/SLPVectorizer/{bbi-107513.ll => isCommutative.ll} (85%) diff --git a/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll b/llvm/test/Transforms/SLPVectorizer/isCommutative.ll similarity index 85% rename from llvm/test/Transforms/SLPVectorizer/bbi-107513.ll rename to llvm/test/Transforms/SLPVectorizer/isCommutative.ll index 14ead464943de..704ac8295f55b 100644 --- a/llvm/test/Transforms/SLPVectorizer/bbi-107513.ll +++ b/llvm/test/Transforms/SLPVectorizer/isCommutative.ll @@ -1,8 +1,8 @@ ; 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( +define i16 @check_isCommutative_with_the_original_source() { +; CHECK-LABEL: @check_isCommutative_with_the_original_source( ; CHECK-NEXT: entry: ; CHECK-NEXT: [[COND3:%.*]] = select i1 true, i16 1, i16 0 ; CHECK-NEXT: ret i16 [[COND3]]