[LV] Check if the VF is scalar by VFRange in handleUncountableEarlyExit.
#135294
Conversation
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
…UncountableEarlyExit` NFC. This patch check if the plan contains scalar VF by VFRange instead of Plan. Then we can furthur clamp the range after this transformation. Split by llvm#113903.
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Elvis Wang (ElvisWang123) ChangesThis patch check if the plan contains scalar VF by VFRange instead of Plan. Then we can further clamp the range after this transformation. Split from #113903. Full diff: https://github.com/llvm/llvm-project/pull/135294.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index a4d546f698d5f..0cec4c5c87f67 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -9744,7 +9744,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
Legal->getUncountableEarlyExitingBlock()) {
VPlanTransforms::runPass(VPlanTransforms::handleUncountableEarlyExit, *Plan,
*PSE.getSE(), OrigLoop, UncountableExitingBlock,
- RecipeBuilder);
+ RecipeBuilder, Range);
}
DenseMap<VPValue *, VPValue *> IVEndValues;
addScalarResumePhis(RecipeBuilder, *Plan, IVEndValues);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 67a55aa67c978..b49a0c452fa1e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -17,6 +17,7 @@
#include "VPlanAnalysis.h"
#include "VPlanCFG.h"
#include "VPlanDominatorTree.h"
+#include "VPlanHelpers.h"
#include "VPlanPatternMatch.h"
#include "VPlanUtils.h"
#include "VPlanVerifier.h"
@@ -2380,7 +2381,7 @@ void VPlanTransforms::convertToConcreteRecipes(VPlan &Plan) {
void VPlanTransforms::handleUncountableEarlyExit(
VPlan &Plan, ScalarEvolution &SE, Loop *OrigLoop,
- BasicBlock *UncountableExitingBlock, VPRecipeBuilder &RecipeBuilder) {
+ BasicBlock *UncountableExitingBlock, VPRecipeBuilder &RecipeBuilder, VFRange &Range) {
VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion();
auto *LatchVPBB = cast<VPBasicBlock>(LoopRegion->getExiting());
VPBuilder Builder(LatchVPBB->getTerminator());
@@ -2436,7 +2437,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
ExitIRI->extractLastLaneOfOperand(MiddleBuilder);
}
// Add the incoming value from the early exit.
- if (!IncomingFromEarlyExit->isLiveIn() && !Plan.hasScalarVFOnly()) {
+ if (!IncomingFromEarlyExit->isLiveIn() && !Range.Start.isScalar()) {
VPValue *FirstActiveLane = EarlyExitB.createNaryOp(
VPInstruction::FirstActiveLane, {EarlyExitTakenCond}, nullptr,
"first.active.lane");
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
index c23ff38265670..85649367c3478 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
@@ -28,6 +28,7 @@ class PredicatedScalarEvolution;
class TargetLibraryInfo;
class VPBuilder;
class VPRecipeBuilder;
+class VFRange;
extern cl::opt<bool> VerifyEachVPlan;
@@ -174,7 +175,8 @@ struct VPlanTransforms {
static void handleUncountableEarlyExit(VPlan &Plan, ScalarEvolution &SE,
Loop *OrigLoop,
BasicBlock *UncountableExitingBlock,
- VPRecipeBuilder &RecipeBuilder);
+ VPRecipeBuilder &RecipeBuilder,
+ VFRange &Range);
/// Lower abstract recipes to concrete ones, that can be codegen'd.
static void convertToConcreteRecipes(VPlan &Plan);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
sdesmalen-arm
approved these changes
Apr 11, 2025
| @@ -2436,7 +2438,7 @@ void VPlanTransforms::handleUncountableEarlyExit( | |||
| ExitIRI->extractLastLaneOfOperand(MiddleBuilder); | |||
| } | |||
| // Add the incoming value from the early exit. | |||
| if (!IncomingFromEarlyExit->isLiveIn() && !Plan.hasScalarVFOnly()) { | |||
| if (!IncomingFromEarlyExit->isLiveIn() && !Range.Start.isScalar()) { | |||
There was a problem hiding this comment.
One more thing; should it additionally check that the Range only has a single VF (i.e. distance(start, end) == 1)?
There was a problem hiding this comment.
This will need to clamp the range to only contain the scalar VF, otherwise we may end up with a plan that is missing the extract and VF > 1
fhahn
requested changes
Apr 11, 2025
| @@ -2436,7 +2438,7 @@ void VPlanTransforms::handleUncountableEarlyExit( | |||
| ExitIRI->extractLastLaneOfOperand(MiddleBuilder); | |||
| } | |||
| // Add the incoming value from the early exit. | |||
| if (!IncomingFromEarlyExit->isLiveIn() && !Plan.hasScalarVFOnly()) { | |||
| if (!IncomingFromEarlyExit->isLiveIn() && !Range.Start.isScalar()) { | |||
There was a problem hiding this comment.
This will need to clamp the range to only contain the scalar VF, otherwise we may end up with a plan that is missing the extract and VF > 1
fhahn
reviewed
Apr 13, 2025
| VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||
| auto *LatchVPBB = cast<VPBasicBlock>(LoopRegion->getExiting()); | ||
| VPBuilder Builder(LatchVPBB->getTerminator()); | ||
| auto *MiddleVPBB = Plan.getMiddleBlock(); | ||
| VPValue *IsEarlyExitTaken = nullptr; | ||
|
|
||
| // Clamp the range that make sure we insert extractElement for incoming value |
There was a problem hiding this comment.
Suggested change
| // Clamp the range that make sure we insert extractElement for incoming value | |
| // Limit range to scalar VF only, if the range contains the scalar VF. |
Also, would be good to move to the use, otherwise we may clamp unnecessarily if no extract needs to be inserted>
There was a problem hiding this comment.
updated, thanks!
That request means the PR would no longer be NFC, right? Is it possible to construct a test for this case? (also 'NFC' would need removing from the title) |
fhahn
reviewed
Apr 14, 2025
There was a problem hiding this comment.
LGTM, thanks.
Please update the description/title before landing. Could also drop instead of VPlan to have a more compact title.
This will need to clamp the range to only contain the scalar VF, otherwise we may end up with a plan that is missing the extract and VF > 1
That request means the PR would no longer be NFC, right? Is it possible to construct a test for this case? (also 'NFC' would need removing from the title)
I am not sure if it is possible to construct one at the moment, due to various restrictions for early-exit vectorization. Not generating the extract for a vector VF included in the range would lead to a silent mis-compile, as without clamping the range we cannot guarantee that the vector VF would not be selected.
ElvisWang123
changed the title
handleUncountableEarlyExit NFC.handleUncountableEarlyExit.
There was a problem hiding this comment.
I am not sure if it is possible to construct one at the moment, due to various restrictions for early-exit vectorization. Not generating the extract for a vector VF included in the range would lead to a silent mis-compile, as without clamping the range we cannot guarantee that the vector VF would not be selected.
Fair enough, thanks for explaining!
Left just one more nit, but otherwise LGTM.
david-arm
reviewed
Apr 15, 2025
| // Add the incoming value from the early exit. | ||
| if (!IncomingFromEarlyExit->isLiveIn() && !Plan.hasScalarVFOnly()) { | ||
| if (!IncomingFromEarlyExit->isLiveIn() && | ||
| // Limit range to scalar VF only, if the range contains the scalar VF. |
There was a problem hiding this comment.
Should this say // Limit range to vector VF only ...? The code below only works if we're dealing with vectors.
There was a problem hiding this comment.
I'm not sure if using Limit range to vector VF only ... will be more confusing or not. Since it won't change the range when it only contains vector VFs and will limit the range to scalar VF when it contains scalar VF.
Perhaps using Limit range to scalar VF only and not insert extract, if the range contains the scalar VF is better?
There was a problem hiding this comment.
The code below if (.. & getDecisionAndClampRange(IsVector, Range)) only applies to vectors and so only applies if Range does not contain any scalar VF. Perhaps you can say that the entire transform is different for vector VFs, as it requires an additional extract element, and so the range needs clamping.
There was a problem hiding this comment.
Updated, thanks for advice!
fhahn
approved these changes
Apr 17, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/14615 Here is the relevant piece of the build log for the reference |
|
@ElvisWang123 I've landed f0621b3 to fix the warning from this PR. Thanks! |
|
@kazutakahirata Thanks! |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…xit`. (llvm#135294) This patch check if the plan contains scalar VF by VFRange instead of Plan. This patch also clamp the range to contains either only scalar or only vector VFs to prevent mis-compile. Split from llvm#113903.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…xit`. (llvm#135294) This patch check if the plan contains scalar VF by VFRange instead of Plan. This patch also clamp the range to contains either only scalar or only vector VFs to prevent mis-compile. Split from llvm#113903.
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.
This patch check if the plan contains scalar VF by VFRange instead of Plan.
This patch also clamp the range to contains either only scalar or only vector VFs to prevent mis-compile.
Split from #113903.