Skip to content

[VPlan] Remove VPWidenRecipe constructor with no underlying instruction. NFCI #166521

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+10 −13
Open

[VPlan] Remove VPWidenRecipe constructor with no underlying instruction. NFCI #166521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c047d9
[VPlan] Remove VPWidenRecipe constructor with no underlying instructi…
lukel97 Nov 5, 2025
1a6bec9
Add transferMetadata
lukel97 Nov 6, 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
6 changes: 3 additions & 3 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8641,9 +8641,9 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
CurrentLinkI->getOpcode() == Instruction::Sub) {
Type *PhiTy = PhiR->getUnderlyingValue()->getType();
auto *Zero = Plan->getConstantInt(PhiTy, 0);
VPWidenRecipe *Sub = new VPWidenRecipe(
Instruction::Sub, {Zero, CurrentLink->getOperand(1)}, {},
VPIRMetadata(), CurrentLinkI->getDebugLoc());
auto *Sub = new VPInstruction(Instruction::Sub,
{Zero, CurrentLink->getOperand(1)},
CurrentLinkI->getDebugLoc());
Sub->setUnderlyingValue(CurrentLinkI);
LinkVPBB->insert(Sub, CurrentLink->getIterator());
VecOp = Sub;
Expand Down
17 changes: 7 additions & 10 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,9 +962,12 @@ class VPIRMetadata {
Metadata.emplace_back(Kind, Node);
}

/// Intersect this VPIRMetada object with \p MD, keeping only metadata
/// Intersect this VPIRMetadata object with \p MD, keeping only metadata
/// nodes that are common to both.
void intersect(const VPIRMetadata &MD);

/// Replace all metadata with \p MD.
void transferMetadata(const VPIRMetadata &MD) { Metadata = MD.Metadata; }
};

/// This is a concrete Recipe that models a single VPlan-level instruction.
Expand Down Expand Up @@ -1447,22 +1450,16 @@ class LLVM_ABI_FOR_TEST VPWidenRecipe : public VPRecipeWithIRFlags,
unsigned Opcode;

public:
VPWidenRecipe(unsigned Opcode, ArrayRef<VPValue *> Operands,
const VPIRFlags &Flags, const VPIRMetadata &Metadata,
DebugLoc DL)
: VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, Flags, DL),
VPIRMetadata(Metadata), Opcode(Opcode) {}

VPWidenRecipe(Instruction &I, ArrayRef<VPValue *> Operands)
: VPRecipeWithIRFlags(VPDef::VPWidenSC, Operands, I), VPIRMetadata(I),
Opcode(I.getOpcode()) {}

~VPWidenRecipe() override = default;

VPWidenRecipe *clone() override {
auto *R =
new VPWidenRecipe(getOpcode(), operands(), *this, *this, getDebugLoc());
R->setUnderlyingValue(getUnderlyingValue());
auto *R = new VPWidenRecipe(*getUnderlyingInstr(), operands());
Copy link
Contributor Author

@lukel97 lukel97 Nov 5, 2025

Choose a reason for hiding this comment

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

I did a quick check and the metadata doesn't ever seem to change from the underlying instruction. So I think it's fine to just let the Instruction constructor set the VPIRMetadata. The flags change however so we need to transfer that.

Copy link
Contributor

@fhahn fhahn Nov 5, 2025

Choose a reason for hiding this comment

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

Not now perhaps, but relying on that seems very dangerous and can easily break in the future and lead to subtle mis-compiles (e.g. we may have to remove metadata when hoisting)

Copy link
Contributor Author

@lukel97 lukel97 Nov 6, 2025

Choose a reason for hiding this comment

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

I've transferred the metadata over in 1a6bec9. I think VPWidenIntrinsicRecipe/VPWidenCallRecipe/VPWidenSelectRecipe seem to have the same issue, which would be good to fix in another PR

Copy link
Contributor

@fhahn fhahn Nov 6, 2025

Choose a reason for hiding this comment

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

I guess passing via the constructor would require more changes, right? if so the current approach is fine. I am currently working on propagating the metadata from initial VPInstructions, which should hopefully unify this soon

R->transferFlags(*this);
R->transferMetadata(*this);
return R;
}

Expand Down