[SLP] Exit early if MaxVF < MinVF (NFCI). #83283

fhahn · 2024-02-28T16:08:29Z

Exit early if MaxVF < MinVF. In that case, the loop body below will never get entered. Note that this adjusts the condition from MaxVF <= MinVF. If MaxVF == MinVF, vectorization may still be feasible (and the loop below gets entered).

llvmbot · 2024-02-28T16:09:03Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Exit early if MaxVF < MinVF. In that case, the loop body below will never get entered. Note that this adjusts the condition from MaxVF <= MinVF. If MaxVF == MinVF, vectorization may still be feasible (and the loop below gets entered).

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+2-1)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 2b7d518c1c1a78..e381cd2c5794b1 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -13912,10 +13912,11 @@ bool SLPVectorizerPass::vectorizeStores(ArrayRef<StoreInst *> Stores,
       unsigned MinVF = TTI->getStoreMinimumVF(
           R.getMinVF(DL->getTypeSizeInBits(ValueTy)), StoreTy, ValueTy);
 
-      if (MaxVF <= MinVF) {
+      if (MaxVF < MinVF) {
         LLVM_DEBUG(dbgs() << "SLP: Vectorization infeasible as MaxVF (" << MaxVF
                           << ") <= "
                           << "MinVF (" << MinVF << ")\n");
+        return;
       }
 
       // FIXME: Is division-by-2 the correct step? Should we assert that the

RKSimon · 2024-02-28T16:13:32Z

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

@@ -13912,10 +13912,11 @@ bool SLPVectorizerPass::vectorizeStores(ArrayRef<StoreInst *> Stores,
      unsigned MinVF = TTI->getStoreMinimumVF(
          R.getMinVF(DL->getTypeSizeInBits(ValueTy)), StoreTy, ValueTy);

-      if (MaxVF <= MinVF) {
+      if (MaxVF < MinVF) {
        LLVM_DEBUG(dbgs() << "SLP: Vectorization infeasible as MaxVF (" << MaxVF
                          << ") <= "


<= -> <

Should be fixed, thanks!

alexey-bataev · 2024-02-28T16:20:17Z

Hmm, I don't think this is NFC, would be good to have a test

fhahn · 2024-02-28T16:21:44Z

Hmm, I don't think this is NFC, would be good to have a test

Yes I think technically this could change dbg output. Let me add a test for that.

alexey-bataev · 2024-02-28T16:41:05Z

Hmm, I don't think this is NFC, would be good to have a test

Yes I think technically this could change dbg output. Let me add a test for that.

Not a debug, I think it may change vectorization output

…vDist.

fhahn · 2024-02-28T21:56:20Z

Hmm, I don't think this is NFC, would be good to have a test

Yes I think technically this could change dbg output. Let me add a test for that.

Not a debug, I think it may change vectorization output

Ah yes, I missed the updates in the outer loop. I tried to find a test case where the return changes behavior, but wasn't able to do so on X86/AArch64 in MultiSource/SPEC and a set of internal benchmarks.

But I updated the PR to use continue and ScopeExit to make sure Operands/PrevDist are updated accordingly on all early continues

github-actions · 2024-02-28T21:56:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

alexey-bataev · 2024-02-29T14:15:43Z

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

+      // Capturing structured bindings are a C++ 20 extension, so use a separate
+      // variable for now.
+      auto &DataVar = Data;
+      auto E = make_scope_exit([&]() {
        Operands.clear();
-        Operands.push_back(Stores[Data.first]);
-        PrevDist = Data.second;
+        Operands.push_back(Stores[DataVar.first]);
+        PrevDist = DataVar.second;
+      });


Better to make it in a separate NFC patch.

alexey-bataev · 2024-02-29T14:16:46Z

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

@@ -13912,10 +13918,11 @@ bool SLPVectorizerPass::vectorizeStores(ArrayRef<StoreInst *> Stores,
      unsigned MinVF = TTI->getStoreMinimumVF(
          R.getMinVF(DL->getTypeSizeInBits(ValueTy)), StoreTy, ValueTy);

-      if (MaxVF <= MinVF) {
+      if (MaxVF < MinVF) {


I mean this change increases cases, where the vectorization may be triggered, since you're loosening the restriction. So need to add the test for such a case.

I think I might be missing something here. The original code only guarded the debug output by this check AFAICT, but always also reached the for loop below, which would not be entered if MaxVF < MinVF.

Now the code continues early instead of reaching the for loop, so it should only restrict the cases where we execute the loop below (and could vectorize)?

Ah, Yes, missed there were continue before.

Use ScopeExit to make sure Operands/PrevDist are updated on all paths in the loop. This makes it easier to ensure they are updated correctly if new early continues are added. Split off from llvm#83283

…83490) Use ScopeExit to make sure Operands/PrevDist are updated on all paths in the loop. This makes it easier to ensure they are updated correctly if new early continues are added. Split off from #83283 PR: #83490

…-exit

alexey-bataev

LG

[SLP] Exit early if MaxVF < MinVF (NFCI).

3eacfa6

Exit early if MaxVF < MinVF. In that case, the loop body below will never get entered. Note that this adjusts the condition from MaxVF <= MinVF. If MaxVF == MinVF, vectorization may still be feasible (and the loop below gets entered).

fhahn requested review from RKSimon and alexey-bataev February 28, 2024 16:08

llvmbot added vectorization llvm:transforms labels Feb 28, 2024

RKSimon reviewed Feb 28, 2024

View reviewed changes

fhahn mentioned this pull request Feb 28, 2024

[SLP] Collect candidate VFs in vector in vectorizeStores (NFC). #82793

Merged

!fixup <= -> < in debug output

2cf3548

!fixup use continue instead return, scope exit to update Operands/Pre…

7e52dbb

…vDist.

!fixup fix formatting.

89c1415

alexey-bataev reviewed Feb 29, 2024

View reviewed changes

fhahn mentioned this pull request Feb 29, 2024

[SLP] Use ScopeExit to update Operands/PrevDist on all paths. (NFC) #83490

Merged

Merge remote-tracking branch 'origin/main' into users/fhahn/slp-early…

63b5482

…-exit

alexey-bataev approved these changes Mar 1, 2024

View reviewed changes

fhahn merged commit 6fe60bd into main Mar 1, 2024
3 of 4 checks passed

fhahn deleted the users/fhahn/slp-early-exit branch March 1, 2024 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SLP] Exit early if MaxVF < MinVF (NFCI). #83283

[SLP] Exit early if MaxVF < MinVF (NFCI). #83283

fhahn commented Feb 28, 2024

llvmbot commented Feb 28, 2024

RKSimon Feb 28, 2024

fhahn Feb 28, 2024

alexey-bataev commented Feb 28, 2024

fhahn commented Feb 28, 2024

alexey-bataev commented Feb 28, 2024

fhahn commented Feb 28, 2024

github-actions bot commented Feb 28, 2024 •

edited

alexey-bataev Feb 29, 2024

alexey-bataev Feb 29, 2024

fhahn Mar 1, 2024

alexey-bataev Mar 1, 2024

alexey-bataev left a comment

[SLP] Exit early if MaxVF < MinVF (NFCI). #83283

[SLP] Exit early if MaxVF < MinVF (NFCI). #83283

Conversation

fhahn commented Feb 28, 2024

llvmbot commented Feb 28, 2024

RKSimon Feb 28, 2024

Choose a reason for hiding this comment

fhahn Feb 28, 2024

Choose a reason for hiding this comment

alexey-bataev commented Feb 28, 2024

fhahn commented Feb 28, 2024

alexey-bataev commented Feb 28, 2024

fhahn commented Feb 28, 2024

github-actions bot commented Feb 28, 2024 • edited

alexey-bataev Feb 29, 2024

Choose a reason for hiding this comment

alexey-bataev Feb 29, 2024

Choose a reason for hiding this comment

fhahn Mar 1, 2024

Choose a reason for hiding this comment

alexey-bataev Mar 1, 2024

Choose a reason for hiding this comment

alexey-bataev left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2024 •

edited