[PATCH] D129436: [LV] Use PHI recipe instead of PredRecipe for subsequent uses.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8409
                                        IsUniform, IsPredicated);
-  setRecipe(I, Recipe);
   Plan->addVPValue(I, Recipe);
 
----------------
This addVPValue was next to setRecipe; could it too sink to remain next to it?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8448
 VPRegionBlock *VPRecipeBuilder::createReplicateRegion(
     Instruction *Instr, VPReplicateRecipe *PredRecipe, VPlanPtr &Plan) {
   // Instructions marked for predication are replicated and placed under an
----------------
nit: can avoid passing Instr given that it's underlying PredRecipe.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8449
+    Plan->removeVPValueFor(Instr);
+    Plan->addVPValue(Instr, PHIRecipe);
+  } else
----------------
fhahn wrote:
> Ayal wrote:
> > Re-associating Instr with PHIRecipe instead of some previous VPValue (i.e., PredRecipe) via removeVPValueFor()/addVPValue() in order to affect subsequent get[OrAdd]VPValue()'s, seems aligned with re-associating setRecipe() to affect subsequent getRecipe()'s. Should we retain the initial setRecipe() which aligns with initial addVPValue(), and introduce the ability to resetRecipe() here? Can also replace removeVPValue()(+addVPValue()) with replaceVPValue(). The other (two) instances of removeVPValue()/addVPValue() pairs, namely adjustRecipesForReductions() and when transforming InterleaveGroups, should not resetRecipe(), right?
> > 
> > 
> Adding a `resetRecipe` would be an option, but resetting a VPValue/recipe should only be an exception. In this case it seems cleaner to just delay setting it. WDYT?
Agreed, but would be good to align (re)setRecipe with (remove)addVPValue; could the latter also be delayed, considering the cleaned-up call to getOrAddVPValue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129436