Skip to content

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 19, 2024
Merged

[LV] Check isPredInst instead of isScalarWithPred in uniform analysis. #98892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6ab904
[LV] Check isPredInst instead of isScalarWithPred in uniform analysis.
fhahn Jul 15, 2024
2a40a3c
Merge remote-tracking branch 'origin/main' into lv-uniform-predinst
fhahn Jul 18, 2024
def9add
Merge remote-tracking branch 'origin/main' into lv-uniform-predinst
fhahn Jul 19, 2024
e3293bb
!fixup address comments, thanks!
fhahn Jul 19, 2024
2afd499
Merge remote-tracking branch 'origin/main' into lv-uniform-predinst
fhahn Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3914,19 +3914,19 @@ void LoopVectorizationCostModel::collectLoopUniforms(ElementCount VF) {
SetVector<Instruction *> Worklist;

// Add uniform instructions demanding lane 0 to the worklist. Instructions
// that are scalar with predication must not be considered uniform after
// that require predication must not be considered uniform after
// vectorization, because that would create an erroneous replicating region
// where only a single instance out of VF should be formed.
// TODO: optimize such seldom cases if found important, see PR40816.
auto addToWorklistIfAllowed = [&](Instruction *I) -> void {
if (isOutOfScope(I)) {
LLVM_DEBUG(dbgs() << "LV: Found not uniform due to scope: "
<< *I << "\n");
return;
}
if (isScalarWithPredication(I, VF)) {
LLVM_DEBUG(dbgs() << "LV: Found not uniform being ScalarWithPredication: "
<< *I << "\n");
if (isPredicatedInst(I)) {
LLVM_DEBUG(
dbgs() << "LV: Found not uniform due to requiring predication: " << *I
<< "\n");
return;
}
LLVM_DEBUG(dbgs() << "LV: Found uniform instruction: " << *I << "\n");
Expand Down Expand Up @@ -9516,6 +9516,8 @@ void VPInterleaveRecipe::execute(VPTransformState &State) {
void VPReplicateRecipe::execute(VPTransformState &State) {
Instruction *UI = getUnderlyingInstr();
if (State.Instance) { // Generate a single instance.
assert((State.VF.isScalar() || !isUniform()) &&
"uniform recipe shouldn't be predicated");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this assert be committed separately?

Better catch this earlier, say assert (!IsUniform || !IsPredicated) upon replicate recipe construction and/or at handleReplication()?

Note the if (IsUniform) below - would be good to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 3717776 in handleReplication. There is some additional code there that converts predicated assumes to uniform for scalable vectors which may should be revisited. (Predicated assumes are later removed by a VPlan-to-VPlan transform)

assert(!State.VF.isScalable() && "Can't scalarize a scalable vector");
State.ILV->scalarizeInstruction(UI, this, *State.Instance, State);
// Insert scalar instance packing it into a vector.
Expand Down
Loading
Loading