Skip to content

[VPlan] Assign custom opcodes to recipes not mapping to IR opcodes. #162267

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 4 commits into from
Oct 13, 2025
Merged

[VPlan] Assign custom opcodes to recipes not mapping to IR opcodes. #162267

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfcbf78
[VPlan] Assign custom opcodes to recipes not mapping to IR opcodes.
fhahn Oct 7, 2025
6d74fd4
Merge remote-tracking branch 'origin/main' into vplan-cse-opcode-vpdefid
fhahn Oct 8, 2025
1a28ac3
!fixup adjust whitespace
fhahn Oct 8, 2025
74eee17
Merge branch 'main' into vplan-cse-opcode-vpdefid
fhahn Oct 13, 2025
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
1 change: 1 addition & 0 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,7 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags,
ResumeForEpilogue,
/// Returns the value for vscale.
VScale,
OpsEnd = VScale,
};

/// Returns true if this VPInstruction generates scalar values for all lanes.
Expand Down
16 changes: 11 additions & 5 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1982,6 +1982,13 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> {
.Case<VPWidenIntrinsicRecipe>([](auto *I) {
return std::make_pair(true, I->getVectorIntrinsicID());
})
.Case<VPVectorPointerRecipe>([](auto *I) {
// For recipes that do not directly map to LLVM IR instructions,
// assign opcodes after the last VPInstruction opcode (which is also
// after the last IR Instruction opcode), based on the VPDefID.
return std::make_pair(false,
VPInstruction::OpsEnd + 1 + I->getVPDefID());
Copy link
Collaborator

@ayalz ayalz Oct 13, 2025

Choose a reason for hiding this comment

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

Alternatively, an opcode can be added for VPVectorPointerRecipe, similar to various VPInstructions. OTOH, this offers a systematic way of providing opcodes to all recipes, aiming to also support VPPredInstPHI. Having a universal opcode across all recipes would require addressing multiple recipes with potentially common underlying opcodes.

Copy link
Contributor

@artagnon artagnon Oct 13, 2025

Choose a reason for hiding this comment

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

I initially added the GEP opcode to VPVectorPointerRecipe, but @fhahn said that it other users could potentially conflate a VectorPointer with a plain GEP, causing confusion. I think adding opcodes to the remaining recipes as OpsEnd + 1 + getVPDefID() could be interesting.

})
.Default([](auto *) { return std::nullopt; });
}

Expand All @@ -2005,12 +2012,9 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> {
static bool canHandle(const VPSingleDefRecipe *Def) {
// We can extend the list of handled recipes in the future,
// provided we account for the data embedded in them while checking for
// equality or hashing. We assign VPVectorEndPointerRecipe the GEP opcode,
Copy link
Collaborator

@ayalz ayalz Oct 13, 2025

Choose a reason for hiding this comment

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

So this potentially changes behavior rather than purely NFC, as VPVectorEndPointerRecipe and GEP are now distinct?

Copy link
Contributor

@artagnon artagnon Oct 13, 2025

Choose a reason for hiding this comment

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

We check the subclass ID in addition to the opcode while performing CSE, so this change would be non-functional.

Copy link
Collaborator

@ayalz ayalz Oct 14, 2025

Choose a reason for hiding this comment

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

Is this checking of subclass ID redundant now, and can be replaced by an assert that the subclass ID's are the same if their opcodes are?

Copy link
Contributor Author

@fhahn fhahn Oct 14, 2025

Choose a reason for hiding this comment

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

It is still needed now, as this prevents replacing for example VPWidenRecipe BinOps with VPReplicateRecipe BinOps. This would be taken care of if we include VPDefID for all opcodes, but we would need some kind of total order for VPDefIDs and their start and end opcodes, for all recipes.

Copy link
Collaborator

@ayalz ayalz Oct 17, 2025

Choose a reason for hiding this comment

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

Understood. Making getOpcode/OrIntrinsicID unique across recipes could also be taken care of by consolidating conflicting recipes, so that information about widening vs. replication (as in the example) is encoded elsewhere, e.g., in the type of the operands, if not in the opcode itself.

// as it is essentially a GEP with different semantics.
auto C = isa<VPVectorPointerRecipe>(Def)
? std::make_pair(false, Instruction::GetElementPtr)
: getOpcodeOrIntrinsicID(Def);
// equality or hashing.

auto C = getOpcodeOrIntrinsicID(Def);
Copy link
Contributor

@artagnon artagnon Oct 7, 2025

Choose a reason for hiding this comment

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

Suggested change
auto C = getOpcodeOrIntrinsicID(Def);
auto C = getOpcodeOrIntrinsicID(Def);

Copy link
Contributor Author

@fhahn fhahn Oct 8, 2025

Choose a reason for hiding this comment

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

Should be adjusted, thanks

// The issue with (Insert|Extract)Value is that the index of the
// insert/extract is not a proper operand in LLVM IR, and hence also not in
// VPlan.
Expand Down Expand Up @@ -2048,6 +2052,8 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> {
vputils::isSingleScalar(L) != vputils::isSingleScalar(R) ||
!equal(L->operands(), R->operands()))
return false;
assert(getOpcodeOrIntrinsicID(L) && getOpcodeOrIntrinsicID(R) &&
"must have valid opcode info for both recipes");
if (auto *LFlags = dyn_cast<VPRecipeWithIRFlags>(L))
if (LFlags->hasPredicate() &&
LFlags->getPredicate() !=
Expand Down
Loading