[PATCH] D154644: [LV] Split off code to create initial VPlan (NFC).
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 3 14:02:14 PDT 2023
fhahn marked 4 inline comments as done.
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9121
+ VPlanPtr Plan = buildInitialVPlanWithVPRecipes(Range, DeadInstructions);
+
----------------
Ayal wrote:
> An alternative refactoring may be to outline the second "Now optimize the initial VPlan" part, rather than the first "build Initial VPlan" part. This could be done perhaps by introducing a new `VPlanTransforms::process(VPlan)` method which manages all passes, similar to the meta-pass `createAndOptimizeReplicateRegions()`.
Adjusted, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9143
// in ways that accessing values using original IR values is incorrect.
Plan->disableValue2VPValue();
----------------
Ayal wrote:
> Should this mark the completion of an initial VPlan, which now fully represents its scope and can be transformed w/o referring to original IR based relations?
Sounds good as a start, updated!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9147
// value and introduce FirstOrderRecurrenceSplice VPInstructions.
if (!VPlanTransforms::adjustFixedOrderRecurrences(*Plan, Builder))
return std::nullopt;
----------------
Ayal wrote:
> This is currently the only transformation which may bail out of building a VPlan entirely. Marking the **optional** completion of an initial VPlan here would enable separating it from transformations that only improve it w/o bailing out, as in having the caller do:
> ```
> if (auto Plan = tryToBuildInitialVPlanWithVPRecipes(SubRange, DeadInstructions)) {
> transformVPlanWithRecipes(*Plan); // Or directly VPlanTransforms::process(*Plan);
> VPlans.push_back(std::move(*Plan));
> }
> ```
> which clearly indicates that `DeadInstructions` and `SubRange` are not used nor modified by the latter transforming phase.
Adjusted, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9156
VPlanTransforms::createAndOptimizeReplicateRegions(*Plan);
----------------
Ayal wrote:
> Note that creating replicate regions is currently the only (meta) pass that is mandatory. It may be useful to have a minimal "-O0" path consisting of vital transforms only, which checks the functional correctness of the outcome.
This might be good in the future, not sure how we best model the options (would also be good to allow running individual optimizations)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154644/new/
https://reviews.llvm.org/D154644
More information about the llvm-commits
mailing list