Repository navigation
[SLP] Fix extract-cost scale for LCSSA-phi external users in nested loops - #199954
Merged
Conversation
Created using spr 1.3.7
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Alexey Bataev (alexey-bataev) ChangesgetScaleToLoopIterations() used U->getParent() for all PHI-node external Full diff: https://github.com/llvm/llvm-project/pull/199954.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 9d7821d2760f2..7c91ffc57cf1a 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -16395,7 +16395,36 @@ uint64_t BoUpSLP::getScaleToLoopIterations(const TreeEntry &TE, Value *Scalar,
Instruction *U) {
BasicBlock *Parent = nullptr;
if (U) {
- Parent = U->getParent();
+ // For PHI node users that are themselves inside a loop, the
+ // extractelement is placed in the incoming block for the scalar
+ // operand, not in the PHI's own block. This matters for LCSSA phis at
+ // an inner-loop exit that is still inside an outer loop: the PHI block
+ // is in the outer loop, but the operand flows from the inner loop body,
+ // so the extract executes at inner-loop frequency. Using the PHI's own
+ // block would underestimate the scale (outer TC instead of inner*outer
+ // TC) and make the ordering of external users affect the cost, since
+ // ExtractCostCalculated deduplicates by scalar and only the first
+ // encountered user determines the scale.
+ // When the PHI is outside all loops (a true loop-exit phi) the extract
+ // is materialised at the phi's location and scale = 1 remains correct,
+ // so we only apply this adjustment when PhiLoop is non-null.
+ if (auto *PHI = dyn_cast<PHINode>(U); PHI && Scalar) {
+ const Loop *PhiLoop = LI->getLoopFor(PHI->getParent());
+ if (PhiLoop) {
+ // Use the deepest incoming block among all slots where Scalar
+ // appears, to be conservative when the same value appears in
+ // multiple predecessors.
+ for (unsigned I = 0, E = PHI->getNumIncomingValues(); I != E; ++I) {
+ if (PHI->getIncomingValue(I) != Scalar)
+ continue;
+ BasicBlock *InBB = PHI->getIncomingBlock(I);
+ if (!Parent || LI->getLoopDepth(InBB) > LI->getLoopDepth(Parent))
+ Parent = InBB;
+ }
+ }
+ }
+ if (!Parent)
+ Parent = U->getParent();
} else if (TE.isGather() || TE.State == TreeEntry::SplitVectorize) {
EdgeInfo EI = TE.UserTreeIndex;
while (EI.UserTE) {
diff --git a/llvm/test/Transforms/SLPVectorizer/AArch64/lcssa-phi-inner-loop-scale.ll b/llvm/test/Transforms/SLPVectorizer/AArch64/lcssa-phi-inner-loop-scale.ll
index 83c4dc891b794..30e3d6b7b70b4 100644
--- a/llvm/test/Transforms/SLPVectorizer/AArch64/lcssa-phi-inner-loop-scale.ll
+++ b/llvm/test/Transforms/SLPVectorizer/AArch64/lcssa-phi-inner-loop-scale.ll
@@ -12,21 +12,25 @@ define void @test_correctness(ptr noalias %p, ptr noalias %q, i32 %n) {
; CHECK-NEXT: [[J:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[J_NEXT:%.*]], %[[OUTER_LATCH:.*]] ]
; CHECK-NEXT: br label %[[INNER_HEADER:.*]]
; CHECK: [[INNER_HEADER]]:
-; CHECK-NEXT: [[I:%.*]] = phi i32 [ 0, %[[OUTER_HEADER]] ], [ [[I_NEXT:%.*]], %[[INNER_BODY:.*]] ]
-; CHECK-NEXT: [[TMP0:%.*]] = phi <2 x i32> [ <i32 10, i32 20>, %[[OUTER_HEADER]] ], [ [[TMP1:%.*]], %[[INNER_BODY]] ]
+; CHECK-NEXT: [[A:%.*]] = phi i32 [ 10, %[[OUTER_HEADER]] ], [ [[A_NEXT:%.*]], %[[INNER_BODY:.*]] ]
+; CHECK-NEXT: [[B:%.*]] = phi i32 [ 20, %[[OUTER_HEADER]] ], [ [[B_NEXT:%.*]], %[[INNER_BODY]] ]
+; CHECK-NEXT: [[I:%.*]] = phi i32 [ 0, %[[OUTER_HEADER]] ], [ [[I_NEXT:%.*]], %[[INNER_BODY]] ]
; CHECK-NEXT: [[COND:%.*]] = icmp ult i32 [[I]], 100
; CHECK-NEXT: br i1 [[COND]], label %[[INNER_BODY]], label %[[OUTER_LATCH]]
; CHECK: [[INNER_BODY]]:
-; CHECK-NEXT: [[TMP1]] = add nsw <2 x i32> [[TMP0]], splat (i32 -1)
+; CHECK-NEXT: [[A_NEXT]] = add nsw i32 [[A]], -1
+; CHECK-NEXT: [[B_NEXT]] = add nsw i32 [[B]], -1
; CHECK-NEXT: [[I_NEXT]] = add nuw nsw i32 [[I]], 1
; CHECK-NEXT: [[I64:%.*]] = zext i32 [[I]] to i64
+; CHECK-NEXT: [[I64N:%.*]] = zext i32 [[I_NEXT]] to i64
; CHECK-NEXT: [[GEP0:%.*]] = getelementptr inbounds i32, ptr [[P]], i64 [[I64]]
-; CHECK-NEXT: store <2 x i32> [[TMP1]], ptr [[GEP0]], align 4
+; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i32, ptr [[P]], i64 [[I64N]]
+; CHECK-NEXT: store i32 [[A_NEXT]], ptr [[GEP0]], align 4
+; CHECK-NEXT: store i32 [[B_NEXT]], ptr [[GEP1]], align 4
; CHECK-NEXT: br label %[[INNER_HEADER]]
; CHECK: [[OUTER_LATCH]]:
-; CHECK-NEXT: [[TMP2:%.*]] = phi <2 x i32> [ [[TMP0]], %[[INNER_HEADER]] ]
-; CHECK-NEXT: [[TMP3:%.*]] = extractelement <2 x i32> [[TMP2]], i32 0
-; CHECK-NEXT: [[TMP4:%.*]] = extractelement <2 x i32> [[TMP2]], i32 1
+; CHECK-NEXT: [[TMP3:%.*]] = phi i32 [ [[A]], %[[INNER_HEADER]] ]
+; CHECK-NEXT: [[TMP4:%.*]] = phi i32 [ [[B]], %[[INNER_HEADER]] ]
; CHECK-NEXT: [[SUM:%.*]] = add i32 [[TMP3]], [[TMP4]]
; CHECK-NEXT: store i32 [[SUM]], ptr [[Q]], align 4
; CHECK-NEXT: [[J_NEXT]] = add nuw nsw i32 [[J]], 1
@@ -87,25 +91,27 @@ define void @test_ordering_lcssa_first(ptr noalias %p, ptr noalias %q, i32 %n) {
; CHECK-NEXT: [[J:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[J_NEXT:%.*]], %[[OUTER_LATCH:.*]] ]
; CHECK-NEXT: br label %[[INNER_HEADER:.*]]
; CHECK: [[INNER_HEADER]]:
-; CHECK-NEXT: [[I:%.*]] = phi i32 [ 0, %[[OUTER_HEADER]] ], [ [[I_NEXT:%.*]], %[[INNER_BODY:.*]] ]
-; CHECK-NEXT: [[TMP0:%.*]] = phi <2 x i32> [ <i32 10, i32 20>, %[[OUTER_HEADER]] ], [ [[TMP3:%.*]], %[[INNER_BODY]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ 10, %[[OUTER_HEADER]] ], [ [[A_NEXT:%.*]], %[[INNER_BODY:.*]] ]
+; CHECK-NEXT: [[TMP2:%.*]] = phi i32 [ 20, %[[OUTER_HEADER]] ], [ [[B_NEXT:%.*]], %[[INNER_BODY]] ]
+; CHECK-NEXT: [[I:%.*]] = phi i32 [ 0, %[[OUTER_HEADER]] ], [ [[I_NEXT:%.*]], %[[INNER_BODY]] ]
; CHECK-NEXT: [[COND:%.*]] = icmp ult i32 [[I]], 100
; CHECK-NEXT: br i1 [[COND]], label %[[INNER_BODY]], label %[[OUTER_LATCH]]
; CHECK: [[INNER_BODY]]:
-; CHECK-NEXT: [[TMP1:%.*]] = extractelement <2 x i32> [[TMP0]], i32 0
; CHECK-NEXT: call void @use(i32 [[TMP1]])
-; CHECK-NEXT: [[TMP2:%.*]] = extractelement <2 x i32> [[TMP0]], i32 1
; CHECK-NEXT: call void @use(i32 [[TMP2]])
-; CHECK-NEXT: [[TMP3]] = add nsw <2 x i32> [[TMP0]], splat (i32 -1)
+; CHECK-NEXT: [[A_NEXT]] = add nsw i32 [[TMP1]], -1
+; CHECK-NEXT: [[B_NEXT]] = add nsw i32 [[TMP2]], -1
; CHECK-NEXT: [[I_NEXT]] = add nuw nsw i32 [[I]], 1
; CHECK-NEXT: [[I64:%.*]] = zext i32 [[I]] to i64
+; CHECK-NEXT: [[I64N:%.*]] = zext i32 [[I_NEXT]] to i64
; CHECK-NEXT: [[GEP0:%.*]] = getelementptr inbounds i32, ptr [[P]], i64 [[I64]]
-; CHECK-NEXT: store <2 x i32> [[TMP3]], ptr [[GEP0]], align 4
+; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i32, ptr [[P]], i64 [[I64N]]
+; CHECK-NEXT: store i32 [[A_NEXT]], ptr [[GEP0]], align 4
+; CHECK-NEXT: store i32 [[B_NEXT]], ptr [[GEP1]], align 4
; CHECK-NEXT: br label %[[INNER_HEADER]]
; CHECK: [[OUTER_LATCH]]:
-; CHECK-NEXT: [[TMP4:%.*]] = phi <2 x i32> [ [[TMP0]], %[[INNER_HEADER]] ]
-; CHECK-NEXT: [[TMP5:%.*]] = extractelement <2 x i32> [[TMP4]], i32 0
-; CHECK-NEXT: [[TMP6:%.*]] = extractelement <2 x i32> [[TMP4]], i32 1
+; CHECK-NEXT: [[TMP5:%.*]] = phi i32 [ [[TMP1]], %[[INNER_HEADER]] ]
+; CHECK-NEXT: [[TMP6:%.*]] = phi i32 [ [[TMP2]], %[[INNER_HEADER]] ]
; CHECK-NEXT: [[SUM:%.*]] = add i32 [[TMP5]], [[TMP6]]
; CHECK-NEXT: store i32 [[SUM]], ptr [[Q]], align 4
; CHECK-NEXT: [[J_NEXT]] = add nuw nsw i32 [[J]], 1
|
RKSimon
reviewed
May 27, 2026
Created using spr 1.3.7
bababuck
reviewed
May 28, 2026
bababuck
reviewed
May 28, 2026
Comment on lines
+16408
to
+16410
| // When the PHI is outside all loops (a true loop-exit phi) the extract | ||
| // is materialised at the phi's location and scale = 1 remains correct, | ||
| // so we only apply this adjustment when PHI is contained in the loop. |
Contributor
There was a problem hiding this comment.
I'm struggling to understand why it matters if the PHI is in an outer loop or not?
Member
Author
There was a problem hiding this comment.
Because the extract for a PHI user is never emitted in the PHI block itself - it is placed in the incoming block of the matching operand (a non-PHI instruction cannot precede the PHIs). So the block whose frequency we must scale by is that incoming block, and it may differ from PHI->getParent().
Contributor
There was a problem hiding this comment.
And it needs to be nested because?
And on a side note, couldn't we do the transformation?
loop:
...
extracted_val = extractelement
loop_exit:
phi [extracted_val, %loop]
to
loop:
...
loop_pre_exit:
extracted_val = extractelement
loop_exit:
phi [extracted_val, %loop_pre_exit]
Member
Author
There was a problem hiding this comment.
No, we can't, not now, at least. It requires special transformation pipeline, which is a separate problem. Plus, in many cases it might be different thing, where we have "expensive" in-loop phi with "cheap" out-of-loop incoming block.
Member
Author
There was a problem hiding this comment.
And it needs to be nested because?
Because incoming block may belong to a different loop (nest) or be outside of the loop at all.
Created using spr 1.3.7
RKSimon
self-requested a review
May 29, 2026 09:09
alexey-bataev
deleted the
users/alexey-bataev/spr/slp-fix-extract-cost-scale-for-lcssa-phi-external-users-in-nested-loops
branch
May 30, 2026 11:26
llvm-upstreamsync Bot
pushed a commit
to qualcomm/cpullvm-toolchain
that referenced
this pull request
May 30, 2026
…in nested loops getScaleToLoopIterations() used U->getParent() for all PHI-node external users. For an LCSSA phi at an inner-loop exit still inside an outer loop, this gave outer-loop scale instead of inner*outer scale. Because ExtractCostCalculated deduplicates by scalar, only the first ExternalUser determines the scale, making the cost order-dependent on use-list ordering (and thus on .ll block ordering). Reviewers: hiraditya, RKSimon, bababuck Pull Request: llvm/llvm-project#199954
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 30, 2026
…in nested loops getScaleToLoopIterations() used U->getParent() for all PHI-node external users. For an LCSSA phi at an inner-loop exit still inside an outer loop, this gave outer-loop scale instead of inner*outer scale. Because ExtractCostCalculated deduplicates by scalar, only the first ExternalUser determines the scale, making the cost order-dependent on use-list ordering (and thus on .ll block ordering). Reviewers: hiraditya, RKSimon, bababuck Pull Request: llvm/llvm-project#199954
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getScaleToLoopIterations() used U->getParent() for all PHI-node external
users. For an LCSSA phi at an inner-loop exit still inside an outer loop,
this gave outer-loop scale instead of inner*outer scale. Because
ExtractCostCalculated deduplicates by scalar, only the first ExternalUser
determines the scale, making the cost order-dependent on use-list ordering
(and thus on .ll block ordering).