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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for following-up with refactoring the excessive tryToBuildVPlanWithVPRecipes!
This raises several thoughts...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9121
 
+  VPlanPtr Plan = buildInitialVPlanWithVPRecipes(Range, DeadInstructions);
+
----------------
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()`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9143
   // in ways that accessing values using original IR values is incorrect.
   Plan->disableValue2VPValue();
 
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9147
   // value and introduce FirstOrderRecurrenceSplice VPInstructions.
   if (!VPlanTransforms::adjustFixedOrderRecurrences(*Plan, Builder))
     return std::nullopt;
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9156
 
   VPlanTransforms::createAndOptimizeReplicateRegions(*Plan);
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154644/new/

https://reviews.llvm.org/D154644