-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Introduce chainUsesScalarValues #158377
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,38 @@ bool vputils::onlyScalarValuesUsed(const VPValue *Def) { | |
[Def](const VPUser *U) { return U->usesScalars(Def); }); | ||
} | ||
|
||
bool vputils::chainUsesScalarValues(const VPValue *Root) { | ||
SmallVector<std::pair<const VPValue *, const VPUser *>> Worklist; | ||
for (const VPUser *V : Root->users()) | ||
Worklist.emplace_back(Root, V); | ||
while (!Worklist.empty()) { | ||
auto [Op, U] = Worklist.pop_back_val(); | ||
if (isa<VPWidenRecipe, VPWidenCastRecipe, VPWidenCallRecipe, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I'm missing something here, but I don't understand why we're ignoring these. Are you trying to work around a problem where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Certain widens cannot be narrowed as they don't have an underlying value that would permit us to turn them into single-scalars: for example, VPWidenCast and VPWidenIntrinsic. All the usesScalars are correct as they are written; I think you meant onlyScalarvaluesUsed in VPlanUtils, which is used by more than just optimizations: it is also used in the execution of recipes, and chainUsesScalars is a version that's suitable for optimizations like narrowToSingleScalars. |
||
VPWidenGEPRecipe, VPWidenSelectRecipe, VPWidenIntrinsicRecipe>(U)) { | ||
const VPValue *Def = cast<VPSingleDefRecipe>(U); | ||
for (const VPUser *V : Def->users()) | ||
Worklist.emplace_back(Def, V); | ||
continue; | ||
} | ||
|
||
// It is always profitable to widen stores, even if the address or stored | ||
// value are single-scalars: checking that the operand is single-scalar in | ||
// WidenStores is an important leaf condition of chainUsesScalarValues, as | ||
// WidenStores have no users, and checking that the address or stored-val is | ||
// single-scalar allows the chains of uses to be determined to use scalar | ||
// values (and can be narowed in an optimization routine, for example). | ||
if (isa<VPWidenStoreRecipe>(U) && vputils::isSingleScalar(Op)) | ||
continue; | ||
|
||
if (auto *VPI = dyn_cast<VPInstruction>(U)) | ||
if (VPI->isVectorToScalar() || VPI->isSingleScalar()) | ||
continue; | ||
if (!U->usesScalars(Op)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like really you could just update the version of usesScalars in the VPInstruction class to return true when isVectorToScalar or isSingleScalar returns true? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, that unfortunately doesn't work: usesScalars is intentionally narrower, as it is used in the execution of VPInstructions; isVectorToScalar and isSingleScalar are special cases, which is why they're different routines in the first place. |
||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr) { | ||
if (auto *Expanded = Plan.getSCEVExpansion(Expr)) | ||
return Expanded; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we already iterating backwards through the VPBB? My understanding is that because we're narrowing the uses before the defs anyway we wouldn't need to look through the chain.
Are we seeing the the diff because we're now checking
isVectorToScalar
?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are iterating backwards through the VPBB, but there is no way to narrow certain Widen-like recipes, as they don't have an underlying value (eg. WidenCast might not have an underlying value). The VPI/Store handling in chainUsesScalarValues is also necessary. Another reason is that the chain may flow into a VPBB we don't handle in our shallow-walk.