[SLP] NFC. Use InstructionsState::valid if users just want to know whether VL has same opcode. #120217

HanKuanChen · 2024-12-17T11:48:53Z

Add assert for InstructionsState::getOpcode.
Use InstructionsState::getOpcode only when necessary.

same opcode.

llvmbot · 2024-12-17T11:49:54Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Han-Kuan Chen (HanKuanChen)

Changes

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+12-15)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index d967813075bb9f..8a6958c3541bd7 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -868,8 +868,8 @@ static bool areCompatibleCmpOps(Value *BaseOp0, Value *BaseOp1, Value *Op0,
          (!isa<Instruction>(BaseOp0) && !isa<Instruction>(Op0) &&
           !isa<Instruction>(BaseOp1) && !isa<Instruction>(Op1)) ||
          BaseOp0 == Op0 || BaseOp1 == Op1 ||
-         getSameOpcode({BaseOp0, Op0}, TLI).getOpcode() ||
-         getSameOpcode({BaseOp1, Op1}, TLI).getOpcode();
+         getSameOpcode({BaseOp0, Op0}, TLI).getMainOp() ||
+         getSameOpcode({BaseOp1, Op1}, TLI).getMainOp();
 }
 
 /// \returns true if a compare instruction \p CI has similar "look" and
@@ -2380,7 +2380,7 @@ class BoUpSLP {
         // Use Boyer-Moore majority voting for finding the majority opcode and
         // the number of times it occurs.
         if (auto *I = dyn_cast<Instruction>(OpData.V)) {
-          if (!OpcodeI || !getSameOpcode({OpcodeI, I}, TLI).getOpcode() ||
+          if (!OpcodeI || !getSameOpcode({OpcodeI, I}, TLI).getMainOp() ||
               I->getParent() != Parent) {
             if (NumOpsWithSameOpcodeParent == 0) {
               NumOpsWithSameOpcodeParent = 1;
@@ -2500,7 +2500,7 @@ class BoUpSLP {
                 // next lane does not build same opcode sequence.
                 (Lns == 2 &&
                  !getSameOpcode({Op, getValue((OpI + 1) % OpE, Ln)}, TLI)
-                      .getOpcode() &&
+                      .getMainOp() &&
                  isa<Constant>(Data.V)))) ||
               // 3. The operand in the current lane is loop invariant (can be
               // hoisted out) and another operand is also a loop invariant
@@ -2509,7 +2509,7 @@ class BoUpSLP {
               // FIXME: need to teach the cost model about this case for better
               // estimation.
               (IsInvariant && !isa<Constant>(Data.V) &&
-               !getSameOpcode({Op, Data.V}, TLI).getOpcode() &&
+               !getSameOpcode({Op, Data.V}, TLI).getMainOp() &&
                L->isLoopInvariant(Data.V))) {
             FoundCandidate = true;
             Data.IsUsed = Data.V == Op;
@@ -2539,7 +2539,7 @@ class BoUpSLP {
                 return true;
               Value *OpILn = getValue(OpI, Ln);
               return (L && L->isLoopInvariant(OpILn)) ||
-                     (getSameOpcode({Op, OpILn}, TLI).getOpcode() &&
+                     (getSameOpcode({Op, OpILn}, TLI).getMainOp() &&
                       allSameBlock({Op, OpILn}));
             }))
           return true;
@@ -4766,7 +4766,7 @@ static bool arePointersCompatible(Value *Ptr1, Value *Ptr2,
           !CompareOpcodes ||
           (GEP1 && GEP2 &&
            getSameOpcode({GEP1->getOperand(1), GEP2->getOperand(1)}, TLI)
-               .getOpcode()));
+               .getMainOp()));
 }
 
 /// Calculates minimal alignment as a common alignment.
@@ -13223,7 +13223,7 @@ BoUpSLP::isGatherShuffledSingleRegisterEntry(
       Value *In1 = PHI1->getIncomingValue(I);
       if (isConstant(In) && isConstant(In1))
         continue;
-      if (!getSameOpcode({In, In1}, *TLI).getOpcode())
+      if (!getSameOpcode({In, In1}, *TLI).getMainOp())
         return false;
       if (cast<Instruction>(In)->getParent() !=
           cast<Instruction>(In1)->getParent())
@@ -13251,7 +13251,7 @@ BoUpSLP::isGatherShuffledSingleRegisterEntry(
     if (It != UsedValuesEntry.end())
       UsedInSameVTE = It->second == UsedValuesEntry.find(V)->second;
     return V != V1 && MightBeIgnored(V1) && !UsedInSameVTE &&
-           getSameOpcode({V, V1}, *TLI).getOpcode() &&
+           getSameOpcode({V, V1}, *TLI).getMainOp() &&
            cast<Instruction>(V)->getParent() ==
                cast<Instruction>(V1)->getParent() &&
            (!isa<PHINode>(V1) || AreCompatiblePHIs(V, V1));
@@ -21346,8 +21346,7 @@ bool SLPVectorizerPass::vectorizeChainsInBlock(BasicBlock *BB, BoUpSLP &R) {
             return false;
           if (I1->getParent() != I2->getParent())
             return false;
-          InstructionsState S = getSameOpcode({I1, I2}, *TLI);
-          if (S.getOpcode())
+          if (getSameOpcode({I1, I2}, *TLI).getMainOp())
             continue;
           return false;
         }
@@ -21701,8 +21700,7 @@ bool SLPVectorizerPass::vectorizeStoreChains(BoUpSLP &R) {
                "Different nodes should have different DFS numbers");
         if (NodeI1 != NodeI2)
           return NodeI1->getDFSNumIn() < NodeI2->getDFSNumIn();
-        InstructionsState S = getSameOpcode({I1, I2}, *TLI);
-        if (S.getOpcode())
+        if (getSameOpcode({I1, I2}, *TLI).getMainOp())
           return false;
         return I1->getOpcode() < I2->getOpcode();
       }
@@ -21728,8 +21726,7 @@ bool SLPVectorizerPass::vectorizeStoreChains(BoUpSLP &R) {
       if (auto *I2 = dyn_cast<Instruction>(V2->getValueOperand())) {
         if (I1->getParent() != I2->getParent())
           return false;
-        InstructionsState S = getSameOpcode({I1, I2}, *TLI);
-        return S.getOpcode() > 0;
+        return getSameOpcode({I1, I2}, *TLI).getMainOp() != nullptr;
       }
     if (isa<Constant>(V1->getValueOperand()) &&
         isa<Constant>(V2->getValueOperand()))

alexey-bataev · 2024-12-18T12:44:19Z

Retitle the patch

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

alexey-bataev · 2024-12-26T16:36:09Z

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

-  if (S.getOpcode() == Instruction::ExtractElement &&
-      isa<ScalableVectorType>(
-          cast<ExtractElementInst>(S.getMainOp())->getVectorOperandType())) {
+  if (auto *EE = dyn_cast_if_present<ExtractElementInst>(S.getMainOp());


Suggested change

if (auto *EE = dyn_cast_if_present<ExtractElementInst>(S.getMainOp());

if (auto *EE = dyn_cast<ExtractElementInst>(S.getMainOp());

Cannot. S may not be valid.

Then you cannot use getMainOp

The origin code check MainOp opcode (S.getOpcode() == Instruction::ExtractElement) and use cast later. I think use dyn_cast_if_present is better.

if (S.getOpcode() == Instruction::ExtractElement && isa<ScalableVectorType>( cast<ExtractElementInst>(S.getMainOp())->getVectorOperandType())) {

There is a case which uses isa_and_present. Follow the same idea, I use dyn_cast_if_present here.

bool AreAllSameInsts = AreAllSameBlock || AreScatterAllGEPSameBlock; if (!AreAllSameInsts || (!S && allConstant(VL)) || isSplat(VL) || (isa_and_present<InsertElementInst, ExtractValueInst, ExtractElementInst>( S.getMainOp()) && !all_of(VL, isVectorLikeInstWithConstOps)) || NotProfitableForVectorization(VL)) {

I.e. the original code check if S is valid, has the opcode ExtractElement and then checks the vector type. getMainOp will crash, if S is invalid. If S is valid, getMainOp cannot return nullptr. dyn_cast_if_present should not be used here, just a dyn_cast should be enough. getMainOp just cannot return nullptr.

It should be fixed, need to replace by S and isa<....>(S.getMainOp()). It worked before, currently it will crash, if S is invalid

Currently some code use isa some code use getOpcode. I think we should either

use isa, dyn_cast, isa_and_present and dyn_cast_if_present for getMainOp

use getOpcode and check whether the opcode is what we want

to make the code consistency.
However, isa (and others) can check isa_and_present<InsertElementInst, ExtractValueInst, ExtractElementInst> with less code. Use getOpcode will write a lot of code in this case.

It won't work, it will crash the compiler, _and_present are not required

It can work. See c9ccb55.
isa_and_present<> = S.valid() && isa<>

It should not work. getMainOp() and getAltOp() should crash if requested from invalid state

OK. See 1c16e5c

…tate-use-getMainOp-2

alexey-bataev

getMainOp and getAltOp should assert if the state is invalid

HanKuanChen · 2025-01-03T14:57:15Z

getMainOp and getAltOp should assert if the state is invalid

Cannot. TreeEntry will use InstructionsState::getMainOp and InstructionsState::getAltOp. But the InstructionsState may be invalid (newTreeEntry may get an invalid InstructionsState).

alexey-bataev · 2025-01-03T15:09:52Z

getMainOp and getAltOp should assert if the state is invalid

Cannot. TreeEntry will use InstructionsState::getMainOp and InstructionsState::getAltOp. But the InstructionsState may be invalid (newTreeEntry may get an invalid InstructionsState).

It must be fixed. TreeEntry shall check if the state is valid and only after using getMainOp and getAltOp. Otherwise, use nullptr

alexey-bataev

LG

pawosm-arm · 2025-01-10T14:55:56Z

I'm seeing this when doing check-llvm:

clang++: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:821: llvm::Instruction* {anonymous}::InstructionsState::getMainOp() const: Assertion `valid() && "InstructionsState is invalid."' failed.

This happens when compiling llvm/unittests/ADT/DenseMapTest.cpp, did anyone else observe that too?

HanKuanChen · 2025-01-10T14:58:29Z

I'm seeing this when doing check-llvm:
clang++: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:821: llvm::Instruction* {anonymous}::InstructionsState::getMainOp() const: Assertion `valid() && "InstructionsState is invalid."' failed.
This happens when compiling llvm/unittests/ADT/DenseMapTest.cpp, did anyone else observe that too?

This may be solved by #122443.
BTW, can you provide the command you use?

pawosm-arm · 2025-01-10T15:13:04Z

Not sure what command you need, check-llvm is a part of the testing I've invoked with ninja check-all. I presume the following would be more helpful for you:

clang++: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:821: llvm::Instruction* {anonymous}::InstructionsState::getMainOp() const: Assertion `valid() && "InstructionsState is invalid."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: bin/clang++ -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_STATIC -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/ADT -Illvm/unittests/ADT -Iinclude -Illvm/include -Ithird-party/unittest/googletest/include -Ithird-party/unittest/googlemock/include -D_LIBCPP_VERBOSE_ABORT_NOT_NOEXCEPT -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-param
eter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -f
diagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++17 -MD -MT unittests/ADT/CMakeFiles/ADTTests.dir/DenseMapTest.cpp.o -MF unittests/ADT/CMakeFiles/ADTTests.di
r/DenseMapTest.cpp.o.d -o unittests/ADT/CMakeFiles/ADTTests.dir/DenseMapTest.cpp.o -c llvm/unittests/ADT/DenseMapTest.cpp
1.      <eof> parser at end of file
2.      Optimizer
3.      Running pass "function<eager-inv>(float2int,lower-constant-intrinsics,chr,loop(loop-rotate<header-duplication;no-prepare-for-lto>,loop-deletion),loop-distribute,inject-tli-mappings,loop-vectorize<no-interleave-forced-only;no-vectorize-forced-only;>,infer-alignment,loop-load-elim,instcombine<max-iterations=1
;no-verify-fixpoint>,simplifycfg<bonus-inst-threshold=1;forward-switch-cond;switch-range-to-icmp;switch-to-lookup;no-keep-loops;hoist-common-insts;no-hoist-loads-stores-with-cond-faulting;sink-common-insts;speculate-blocks;simplify-cond-branch;no-speculate-unpredictables>,slp-vectorizer,vector-combine,instcombine<m
ax-iterations=1;no-verify-fixpoint>,loop-unroll<O3>,transform-warning,sroa<preserve-cfg>,infer-alignment,instcombine<max-iterations=1;no-verify-fixpoint>,loop-mssa(licm<allowspeculation>),alignment-from-assumptions,loop-sink,instsimplify,div-rem-pairs,tailcallelim,simplifycfg<bonus-inst-threshold=1;no-forward-switc
h-cond;switch-range-to-icmp;no-switch-to-lookup;keep-loops;no-hoist-common-insts;hoist-loads-stores-with-cond-faulting;no-sink-common-insts;speculate-blocks;simplify-cond-branch;speculate-unpredictables>)" on module "llvm/unittests/ADT/DenseMapTes
t.cpp"
4.      Running pass "slp-vectorizer" on function "_ZN12_GLOBAL__N_140DenseMapCustomTest_InitFromIterator_Test8TestBodyEv"
 #0 0x0000aaaad1577b64 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (bin/clang+++0x2227b64)
 #1 0x0000aaaad157558c llvm::sys::RunSignalHandlers() (bin/clang+++0x222558c)
 #2 0x0000aaaad14d28a4 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x0000ffffae1fb7dc (linux-vdso.so.1+0x7dc)
 #4 0x0000ffffadd7f200 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x0000ffffadd3a67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000ffffadd27130 abort ./stdlib/abort.c:81:7
 #7 0x0000ffffadd33fd0 __assert_fail_base ./assert/assert.c:89:7
 #8 0x0000ffffadd34040 __assert_perror_fail ./assert/assert-perr.c:31:1
 #9 0x0000aaaad2ea9c68 (bin/clang+++0x3b59c68)
#10 0x0000aaaad2f5ebd4 llvm::slpvectorizer::BoUpSLP::transformNodes() (bin/clang+++0x3c0ebd4)
#11 0x0000aaaad2f7cddc (anonymous namespace)::HorizontalReduction::tryToReduce(llvm::slpvectorizer::BoUpSLP&, llvm::DataLayout const&, llvm::TargetTransformInfo*, llvm::TargetLibraryInfo const&, llvm::AssumptionCache*) SLPVectorizer.cpp:0:0
#12 0x0000aaaad2f7f988 llvm::SLPVectorizerPass::vectorizeHorReduction(llvm::PHINode*, llvm::Instruction*, llvm::BasicBlock*, llvm::slpvectorizer::BoUpSLP&, llvm::SmallVectorImpl<llvm::WeakTrackingVH>&) (bin/clang+++0x3c2f988)
#13 0x0000aaaad2f83b84 llvm::SLPVectorizerPass::vectorizeRootInstruction(llvm::PHINode*, llvm::Instruction*, llvm::BasicBlock*, llvm::slpvectorizer::BoUpSLP&) (.constprop.0) SLPVectorizer.cpp:0:0
#14 0x0000aaaad2f87184 llvm::SLPVectorizerPass::vectorizeChainsInBlock(llvm::BasicBlock*, llvm::slpvectorizer::BoUpSLP&) (bin/clang+++0x3c37184)
#15 0x0000aaaad2f8caa4 llvm::SLPVectorizerPass::runImpl(llvm::Function&, llvm::ScalarEvolution*, llvm::TargetTransformInfo*, llvm::TargetLibraryInfo*, llvm::AAResults*, llvm::LoopInfo*, llvm::DominatorTree*, llvm::AssumptionCache*, llvm::DemandedBits*, llvm::OptimizationRemarkEmitter*) (.part.0) SLPVectorizer.cpp:0
:0

fhahn · 2025-01-10T17:57:43Z

Looks like this has been crashing for a couple of hours.

Can we revert this to unblock people or land a fix ASAP?

HanKuanChen · 2025-01-10T18:02:32Z

Looks like this has been crashing for a couple of hours.

Can we revert this to unblock people or land a fix ASAP?

It is fixed by #122443.

fhahn · 2025-01-10T18:26:13Z

Looks like this has been crashing for a couple of hours.
Can we revert this to unblock people or land a fix ASAP?

It is fixed by #122443.

I see, but the fix hasn't landed yet, and it is not clear when it will land. If it takes longer, the change should be reverted

Also, if the change fixes a crash, I don't think it is really NFC :)

HanKuanChen · 2025-01-11T03:52:27Z

Looks like this has been crashing for a couple of hours.
Can we revert this to unblock people or land a fix ASAP?

It is fixed by #122443.

I see, but the fix hasn't landed yet, and it is not clear when it will land. If it takes longer, the change should be reverted

Also, if the change fixes a crash, I don't think it is really NFC :)

Really appreciate your advice. I will test more and resubmit it. It is reverted already.

[SLP] NFC. Use getMainOp if users just want to know whether VL has

5624823

same opcode.

HanKuanChen requested a review from alexey-bataev December 17, 2024 11:48

llvmbot added vectorizers llvm:transforms labels Dec 17, 2024

add InstructionsState::valid

3b885f2

HanKuanChen changed the title ~~[SLP] NFC. Use getMainOp if users just want to know whether VL has same opcode.~~ [SLP] NFC. Use InstructionsState::valid if users just want to know whether VL has same opcode. Dec 18, 2024

alexey-bataev reviewed Dec 23, 2024

View reviewed changes

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

HanKuanChen added 2 commits December 23, 2024 17:14

apply comment

533b331

[SLP] NFC. Use InstructionsState::getOpcode only when necessary.

1f60b79

HanKuanChen mentioned this pull request Dec 24, 2024

[SLP] NFC. Use InstructionsState::getOpcode only when necessary. #120210

Closed

HanKuanChen requested a review from alexey-bataev December 24, 2024 01:35

alexey-bataev reviewed Dec 24, 2024

View reviewed changes

apply comment

dd65671

alexey-bataev reviewed Dec 26, 2024

View reviewed changes

HanKuanChen added 3 commits December 26, 2024 10:23

Merge remote-tracking branch 'origin/main' into slp-nfc-InstructionsS…

574178a

…tate-use-getMainOp-2

fix 0d6cb0a

5305e37

apply comment

1c16e5c

HanKuanChen requested a review from alexey-bataev January 3, 2025 09:19

alexey-bataev reviewed Jan 3, 2025

View reviewed changes

HanKuanChen requested a review from alexey-bataev January 3, 2025 14:57

apply comment

688e080

alexey-bataev approved these changes Jan 3, 2025

View reviewed changes

HanKuanChen merged commit c50370c into llvm:main Jan 3, 2025
8 checks passed

HanKuanChen deleted the slp-nfc-InstructionsState-use-getMainOp-2 branch January 3, 2025 16:44

	if (auto *EE = dyn_cast_if_present<ExtractElementInst>(S.getMainOp());
	if (auto *EE = dyn_cast<ExtractElementInst>(S.getMainOp());

[SLP] NFC. Use InstructionsState::valid if users just want to know whether VL has same opcode. #120217

[SLP] NFC. Use InstructionsState::valid if users just want to know whether VL has same opcode. #120217

Uh oh!

Conversation

HanKuanChen commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-bataev commented Dec 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-bataev Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

HanKuanChen commented Jan 3, 2025

Uh oh!

alexey-bataev commented Jan 3, 2025

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pawosm-arm commented Jan 10, 2025

Uh oh!

HanKuanChen commented Jan 10, 2025

Uh oh!

pawosm-arm commented Jan 10, 2025

Uh oh!

fhahn commented Jan 10, 2025

Uh oh!

HanKuanChen commented Jan 10, 2025

Uh oh!

fhahn commented Jan 10, 2025

Uh oh!

HanKuanChen commented Jan 11, 2025

Uh oh!

Uh oh!

HanKuanChen commented Dec 17, 2024 •

edited

Loading

llvmbot commented Dec 17, 2024 •

edited

Loading

alexey-bataev Dec 26, 2024 •

edited

Loading