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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8409
                                        IsUniform, IsPredicated);
-  setRecipe(I, Recipe);
   Plan->addVPValue(I, Recipe);
 
----------------
Ayal wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > This addVPValue was next to setRecipe; could it too sink to remain next to it?
> > > Ah yes, this can be sunk, removing the need for the later removeVPValue!
> > Very well!
> This patch deals with setRecipe() being done here too early w/o being reset later, erroneously. addVPValue is also done here too early - whether `I` defines a value or returns void, is predicated or not - but is reset correctly later. It would be good to keep the two together, so if the fix for setRecipe is to postpone it - move addVPValue as well. However, avoiding a redundant addVPValue when `I` returns void calls for a separate patch?
> r. It would be good to keep the two together, so if the fix for setRecipe is to postpone it - move addVPValue as well. However, avoiding a redundant addVPValue when I returns void calls for a separate patch?

Sounds good, I added `addVPValue` in the `else` branch as well below, to be removed separately.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8449
+    Plan->removeVPValueFor(Instr);
+    Plan->addVPValue(Instr, PHIRecipe);
+  } else
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > Ayal wrote:
> > > > > 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?
> > > > In the latest version there's no need for resetting, as the addVPValue can also be delayed until this point.
> > > Even better.
> > > 
> > > Should also Plan->addVPValue(Instr, PredRecipe) here, retaining existing behavior? Or OTOH if Instr->getType()->isVoidTy() perhaps neither addVPValue nor setRecipe are needed for it?
> > >  Plan->addVPValue(Instr, PredRecipe) 
> > 
> > Given that `Instr` doesn't have a result value (due to void type) there is no need to add the VPValue here. I think `setRecipe` is still needed, in case something needs to be moved after the recipe for `Instr`.
> ok, best for this patch to retain current addVPValue behavior and focus on fixing setRecipe bug only? Separating the potential removal of addVPValue for result-less Instr's, and possibly that of setRecipe as well (the two should arguably be merged) to a subsequent patch, as noted above?
Sounds good, I added the `addVPValue` to the `else` branch as well here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129436