[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations. #122241

HanKuanChen · 2025-01-09T10:00:53Z

Replace Cnt and AltIndex with MainOp and AltOp.
Reduce the number of iterations in the for loop.

llvmbot · 2025-01-09T10:01:28Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Han-Kuan Chen (HanKuanChen)

Changes

Replace Cnt and AltIndex with MainOp and AltOp as InstructionsState.
Reduce the number of iterations in the for loop.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+28-32)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index c4582df89213d8..aa12af43dcf95c 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -915,24 +915,22 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
   if (It == VL.end())
     return InstructionsState::invalid();
 
-  Value *V = *It;
+  Instruction *MainOp = cast<Instruction>(*It);
   unsigned InstCnt = std::count_if(It, VL.end(), IsaPred<Instruction>);
-  if ((VL.size() > 2 && !isa<PHINode>(V) && InstCnt < VL.size() / 2) ||
+  if ((VL.size() > 2 && !isa<PHINode>(MainOp) && InstCnt < VL.size() / 2) ||
       (VL.size() == 2 && InstCnt < 2))
     return InstructionsState::invalid();
 
-  bool IsCastOp = isa<CastInst>(V);
-  bool IsBinOp = isa<BinaryOperator>(V);
-  bool IsCmpOp = isa<CmpInst>(V);
-  CmpInst::Predicate BasePred =
-      IsCmpOp ? cast<CmpInst>(V)->getPredicate() : CmpInst::BAD_ICMP_PREDICATE;
-  unsigned Opcode = cast<Instruction>(V)->getOpcode();
+  bool IsCastOp = isa<CastInst>(MainOp);
+  bool IsBinOp = isa<BinaryOperator>(MainOp);
+  bool IsCmpOp = isa<CmpInst>(MainOp);
+  CmpInst::Predicate BasePred = IsCmpOp ? cast<CmpInst>(MainOp)->getPredicate()
+                                        : CmpInst::BAD_ICMP_PREDICATE;
+  Instruction *AltOp = MainOp;
+  unsigned Opcode = MainOp->getOpcode();
   unsigned AltOpcode = Opcode;
-  unsigned AltIndex = std::distance(VL.begin(), It);
 
-  bool SwappedPredsCompatible = [&]() {
-    if (!IsCmpOp)
-      return false;
+  bool SwappedPredsCompatible = IsCmpOp && [&]() {
     SetVector<unsigned> UniquePreds, UniqueNonSwappedPreds;
     UniquePreds.insert(BasePred);
     UniqueNonSwappedPreds.insert(BasePred);
@@ -955,18 +953,18 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
   }();
   // Check for one alternate opcode from another BinaryOperator.
   // TODO - generalize to support all operators (types, calls etc.).
-  auto *IBase = cast<Instruction>(V);
   Intrinsic::ID BaseID = 0;
   SmallVector<VFInfo> BaseMappings;
-  if (auto *CallBase = dyn_cast<CallInst>(IBase)) {
+  if (auto *CallBase = dyn_cast<CallInst>(MainOp)) {
     BaseID = getVectorIntrinsicIDForCall(CallBase, &TLI);
     BaseMappings = VFDatabase(*CallBase).getMappings(*CallBase);
     if (!isTriviallyVectorizable(BaseID) && BaseMappings.empty())
       return InstructionsState::invalid();
   }
   bool AnyPoison = InstCnt != VL.size();
-  for (int Cnt = 0, E = VL.size(); Cnt < E; Cnt++) {
-    auto *I = dyn_cast<Instruction>(VL[Cnt]);
+  // Skip MainOp.
+  while (++It != VL.end()) {
+    auto *I = dyn_cast<Instruction>(*It);
     if (!I)
       continue;
 
@@ -982,11 +980,11 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
       if (Opcode == AltOpcode && isValidForAlternation(InstOpcode) &&
           isValidForAlternation(Opcode)) {
         AltOpcode = InstOpcode;
-        AltIndex = Cnt;
+        AltOp = I;
         continue;
       }
     } else if (IsCastOp && isa<CastInst>(I)) {
-      Value *Op0 = IBase->getOperand(0);
+      Value *Op0 = MainOp->getOperand(0);
       Type *Ty0 = Op0->getType();
       Value *Op1 = I->getOperand(0);
       Type *Ty1 = Op1->getType();
@@ -998,12 +996,12 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
                  isValidForAlternation(InstOpcode) &&
                  "Cast isn't safe for alternation, logic needs to be updated!");
           AltOpcode = InstOpcode;
-          AltIndex = Cnt;
+          AltOp = I;
           continue;
         }
       }
-    } else if (auto *Inst = dyn_cast<CmpInst>(VL[Cnt]); Inst && IsCmpOp) {
-      auto *BaseInst = cast<CmpInst>(V);
+    } else if (auto *Inst = dyn_cast<CmpInst>(I); Inst && IsCmpOp) {
+      auto *BaseInst = cast<CmpInst>(MainOp);
       Type *Ty0 = BaseInst->getOperand(0)->getType();
       Type *Ty1 = Inst->getOperand(0)->getType();
       if (Ty0 == Ty1) {
@@ -1017,24 +1015,23 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
         CmpInst::Predicate SwappedCurrentPred =
             CmpInst::getSwappedPredicate(CurrentPred);
 
-        if ((E == 2 || SwappedPredsCompatible) &&
+        if ((VL.size() == 2 || SwappedPredsCompatible) &&
             (BasePred == CurrentPred || BasePred == SwappedCurrentPred))
           continue;
 
         if (isCmpSameOrSwapped(BaseInst, Inst, TLI))
           continue;
-        auto *AltInst = cast<CmpInst>(VL[AltIndex]);
-        if (AltIndex) {
-          if (isCmpSameOrSwapped(AltInst, Inst, TLI))
+        if (MainOp != AltOp) {
+          if (isCmpSameOrSwapped(cast<CmpInst>(AltOp), Inst, TLI))
             continue;
         } else if (BasePred != CurrentPred) {
           assert(
               isValidForAlternation(InstOpcode) &&
               "CmpInst isn't safe for alternation, logic needs to be updated!");
-          AltIndex = Cnt;
+          AltOp = I;
           continue;
         }
-        CmpInst::Predicate AltPred = AltInst->getPredicate();
+        CmpInst::Predicate AltPred = cast<CmpInst>(AltOp)->getPredicate();
         if (BasePred == CurrentPred || BasePred == SwappedCurrentPred ||
             AltPred == CurrentPred || AltPred == SwappedCurrentPred)
           continue;
@@ -1045,17 +1042,17 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
              "CastInst.");
       if (auto *Gep = dyn_cast<GetElementPtrInst>(I)) {
         if (Gep->getNumOperands() != 2 ||
-            Gep->getOperand(0)->getType() != IBase->getOperand(0)->getType())
+            Gep->getOperand(0)->getType() != MainOp->getOperand(0)->getType())
           return InstructionsState::invalid();
       } else if (auto *EI = dyn_cast<ExtractElementInst>(I)) {
         if (!isVectorLikeInstWithConstOps(EI))
           return InstructionsState::invalid();
       } else if (auto *LI = dyn_cast<LoadInst>(I)) {
-        auto *BaseLI = cast<LoadInst>(IBase);
+        auto *BaseLI = cast<LoadInst>(MainOp);
         if (!LI->isSimple() || !BaseLI->isSimple())
           return InstructionsState::invalid();
       } else if (auto *Call = dyn_cast<CallInst>(I)) {
-        auto *CallBase = cast<CallInst>(IBase);
+        auto *CallBase = cast<CallInst>(MainOp);
         if (Call->getCalledFunction() != CallBase->getCalledFunction())
           return InstructionsState::invalid();
         if (Call->hasOperandBundles() &&
@@ -1085,8 +1082,7 @@ static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
     return InstructionsState::invalid();
   }
 
-  return InstructionsState(cast<Instruction>(V),
-                           cast<Instruction>(VL[AltIndex]));
+  return InstructionsState(MainOp, cast<Instruction>(AltOp));
 }
 
 /// \returns true if all of the values in \p VL have the same type or false

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

alexey-bataev

LG

vitalybuka · 2025-01-10T02:09:12Z

Looks like it's crashing bots https://lab.llvm.org/buildbot/#/builders/55/builds/5319
CC @fmayer

…vm#122241) Replace Cnt and AltIndex with MainOp and AltOp. Reduce the number of iterations in the for loop.

mikaelholmen · 2025-01-28T12:19:16Z

Hi @HanKuanChen and @alexey-bataev ,

The following starts crashing with this "NFC" patch:

opt -passes="slp-vectorizer" bbi-103405.ll -S -o /dev/null -mtriple=thumb7 -mcpu=swift

bbi-103405.ll.gz

alexey-bataev · 2025-01-28T14:01:44Z

Hi @HanKuanChen and @alexey-bataev ,

The following starts crashing with this "NFC" patch:
opt -passes="slp-vectorizer" bbi-103405.ll -S -o /dev/null -mtriple=thumb7 -mcpu=swift
bbi-103405.ll.gz

Fixed in a1ab5b4

mikaelholmen · 2025-01-28T14:23:39Z

Hi @HanKuanChen and @alexey-bataev ,
The following starts crashing with this "NFC" patch:
opt -passes="slp-vectorizer" bbi-103405.ll -S -o /dev/null -mtriple=thumb7 -mcpu=swift
bbi-103405.ll.gz
Fixed in a1ab5b4

I've verified that the crash goes away with the fix.
Thanks!

HanKuanChen added 3 commits January 8, 2025 06:24

rename IBase to MainOp

2a78d8b

rename V to MainOp

1362815

move IsCmpOp out of lambda

de4aee2

HanKuanChen requested a review from alexey-bataev January 9, 2025 10:00

llvmbot added vectorizers llvm:transforms labels Jan 9, 2025

replace AltIndex with AltOp

caacc2e

HanKuanChen force-pushed the slp-getSameOpcode branch from 0a27e6d to 565384c Compare January 9, 2025 14:41

skip the beginning PoisonValue and MainOp

37511d3

HanKuanChen force-pushed the slp-getSameOpcode branch from 565384c to 37511d3 Compare January 9, 2025 14:43

alexey-bataev reviewed Jan 9, 2025

View reviewed changes

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp Show resolved Hide resolved

apply comment

a076a51

HanKuanChen requested a review from alexey-bataev January 9, 2025 16:03

alexey-bataev approved these changes Jan 9, 2025

View reviewed changes

HanKuanChen merged commit 36b423e into llvm:main Jan 10, 2025
6 of 8 checks passed

HanKuanChen deleted the slp-getSameOpcode branch January 10, 2025 01:06

vitalybuka mentioned this pull request Jan 10, 2025

Revert "[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations." #122407

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations. #122241

[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations. #122241

Uh oh!

HanKuanChen commented Jan 9, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

alexey-bataev left a comment

Uh oh!

Uh oh!

vitalybuka commented Jan 10, 2025

Uh oh!

mikaelholmen commented Jan 28, 2025

Uh oh!

alexey-bataev commented Jan 28, 2025

Uh oh!

mikaelholmen commented Jan 28, 2025

Uh oh!

Uh oh!

[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations. #122241

[SLP] NFC. Refactor getSameOpcode and reduce for loop iterations. #122241

Uh oh!

Conversation

HanKuanChen commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vitalybuka commented Jan 10, 2025

Uh oh!

mikaelholmen commented Jan 28, 2025

Uh oh!

alexey-bataev commented Jan 28, 2025

Uh oh!

mikaelholmen commented Jan 28, 2025

Uh oh!

Uh oh!

HanKuanChen commented Jan 9, 2025 •

edited

Loading

llvmbot commented Jan 9, 2025 •

edited

Loading