[PATCH] D84680: [VPlan] Use VPValue def for VPMemoryInstructionRecipe.

[Dave Green via Phabricator via llvm-commits]

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

Thanks. This looks good to me, and I like the direction we are heading towards.

You might of course want to wait a day or two in case others disagree.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6958
+    auto *WidenLoad = new VPWidenMemoryInstructionRecipe(*Load, Addr, Mask);
+    Plan->addVPValue(Load, WidenLoad);
+    return WidenLoad;
----------------
fhahn wrote:
> dmgreen wrote:
> > It might be worth thinking about doing this after the call to RecipeBuilder.tryToCreateWidenRecipe, so that it's done for all recipes at once.
> We should definitely do that, but I'd thought it would be best to do so once we migrated most/all recipes. 
> 
> I updated the code to try to convert the recipe to a VPValue after tryToCreateWidenRecipe. Unfortunately we won't eb able to use isa/cast/dyn_cast until VPRecipeBase inherits from VPValue, because otherwise we would cast between types that are not connected through inheritance. The main advantage would be to have a single place to update after the switch.
The new code looks OK I think, at least in the sort term.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1167
+class VPWidenMemoryInstructionRecipe : public VPRecipeBase, public VPValue {
   VPUser User;
 
----------------
fhahn wrote:
> dmgreen wrote:
> > Would we want to make this inherit from VPUser as well?
> I've pushed a commit updating all classes that have a VPUser member to instead inherit from VPUser, which we should be able to do independently now that VPuser is not a VPValue (for now)
Yeah I saw. Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84680