[PATCH] D154644: [LV] Split off code to create initial VPlan (NFC).

[Florian Hahn via Phabricator via llvm-commits]

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