[PATCH] D154640: [LV] Move all VPlan transforms after initial VPlan construction.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, with some thoughts of possible follow-up...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9078
   adjustRecipesForReductions(cast<VPBasicBlock>(TopRegion->getExiting()), Plan,
                              RecipeBuilder, Range.Start);
 
----------------
(Possible follow-up:) Could `adjustRecipesForReductions()` be refactored to walk the def-use chains of recipes rather than rely on retrieving recipes of recorded `Instructions`, so it could (continue to) be next to `adjust[RecipesFor]FixedOrderRecurrences()`? The latter part of adjustRecipesForReductions() which introduces selects in case of fold-tail is already Value2VPValue-free.

The call to `RecipeBuilder.fixHeaderPhis()` should indeed be part of the initial VPlan construction as it completes the def-use relations of reduction and FOR header phi's, using Value2VPValue. Still, it too could walk header phi recipes instead of relying on the recorded `PhisToFix`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9142
+  VPlanTransforms::removeRedundantCanonicalIVs(*Plan);
+  VPlanTransforms::removeRedundantInductionCasts(*Plan);
+
----------------
This doesn't use Value2VPValue mapping directly, but rather searches for a recipe whose underlying value is equal to an `Instruction` of the recorded `getCastInsts`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154640