Skip to content

[VPlan] Move out canNarrowOps (NFC). #167309

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 3 commits into
base: main
Choose a base branch
from
+29 −15
Open

[VPlan] Move out canNarrowOps (NFC). #167309

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bac893f
[VPlan] Move out canNarrowOps (NFC).
fhahn Jul 11, 2025
471e12b
!fixup restructure op handling a bit
fhahn Nov 10, 2025
be3ecf2
!fixup Ops0 -> OpsI, thanks!
fhahn Nov 19, 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
44 changes: 29 additions & 15 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4268,6 +4268,34 @@ static bool canNarrowLoad(VPWidenRecipe *WideMember0, unsigned OpIdx,
return false;
}

static bool canNarrowOps(ArrayRef<VPValue *> Ops) {
SmallVector<VPValue *> Ops0;
auto *WideMember0 = dyn_cast<VPWidenRecipe>(Ops[0]);
Copy link
Contributor

@david-arm david-arm Nov 10, 2025

Choose a reason for hiding this comment

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

Looks like this doesn't quite match the old code in narrowInterleaveGroups, i.e.

  ... = dyn_cast_or_null<VPWidenRecipe>(
        InterleaveR->getStoredValues()[0]->getDefiningRecipe());

Does it matter? Just a bit worried it's not actually NFC and changing behaviour in a subtle way.

Copy link
Contributor Author

@fhahn fhahn Nov 10, 2025

Choose a reason for hiding this comment

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

This is regarding dyn_cast_or_null<VPWidenRecipe>(....->getDefiningRecipe()) vs dyn_cast<VPWidenRecipe>(...), right?

They should be equivalent, VPWidenRecipe::classof has an overload for VPValue *, which takes care of handling getDefiningRecipe return nullptr

if (!WideMember0)
return false;

for (const auto &[_, V] : enumerate(Ops)) {
auto *R = dyn_cast<VPWidenRecipe>(V);
if (!R || R->getOpcode() != WideMember0->getOpcode() ||
R->getNumOperands() > 2)
return false;
}

for (unsigned Idx = 0; Idx != WideMember0->getNumOperands(); ++Idx) {
Copy link
Contributor

@david-arm david-arm Nov 10, 2025

Choose a reason for hiding this comment

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

Where has this extra for loop come from? It wasn't in the original code.

Copy link
Contributor Author

@fhahn fhahn Nov 10, 2025

Choose a reason for hiding this comment

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

The previous code did not have 2 nested loops, but was processing R->operands() per iteration to check if all operands for a given position could be narrowed to memory operations.

The change re-orders the processing, to check if all first/second operands can be narrowed together.

Previously the code only handled wide ops with loads directly (which meant that they only had a single operand).

The movement here removes the restriction, but is still NFC, as the legality checks are just limited to one level of recursion at the moment.

I restructured the code a bit, to make this a bit clearer hopefully.

Copy link
Contributor

@david-arm david-arm Nov 17, 2025

Choose a reason for hiding this comment

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

OK thanks for explaining. It does seem a bit clearer now.

SmallVector<VPValue *> OpsI;
for (VPValue *Op : Ops)
OpsI.push_back(Op->getDefiningRecipe()->getOperand(Idx));

if (any_of(enumerate(OpsI), [WideMember0, Idx](const auto &P) {
const auto &[OpIdx, OpV] = P;
return !canNarrowLoad(WideMember0, Idx, OpV, OpIdx);
}))
return false;
}

return true;
}

/// Returns true if \p IR is a full interleave group with factor and number of
/// members both equal to \p VF. The interleave group must also access the full
/// vector width \p VectorRegWidth.
Expand Down Expand Up @@ -4441,22 +4469,8 @@ void VPlanTransforms::narrowInterleaveGroups(VPlan &Plan, ElementCount VF,

// Check if all values feeding InterleaveR are matching wide recipes, which
// operands that can be narrowed.
auto *WideMember0 =
dyn_cast_or_null<VPWidenRecipe>(InterleaveR->getStoredValues()[0]);
if (!WideMember0)
if (!canNarrowOps(InterleaveR->getStoredValues()))
return;
for (const auto &[I, V] : enumerate(InterleaveR->getStoredValues())) {
auto *R = dyn_cast_or_null<VPWidenRecipe>(V);
if (!R || R->getOpcode() != WideMember0->getOpcode() ||
R->getNumOperands() > 2)
return;
if (any_of(enumerate(R->operands()),
[WideMember0, Idx = I](const auto &P) {
const auto &[OpIdx, OpV] = P;
return !canNarrowLoad(WideMember0, OpIdx, OpV, Idx);
}))
return;
}
StoreGroups.push_back(InterleaveR);
}

Expand Down