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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:513
   void vectorizeMemoryInstruction(Instruction *Instr, VPTransformState &State,
-                                  VPValue *Addr, VPValue *StoredValue,
-                                  VPValue *BlockInMask);
+
+                                  VPValue *Def, VPValue *Addr,
----------------
dmgreen wrote:
> There's an extra newline here, that looks like it came from a merge conflict.
That's odd, it seems like clang-format did miss this one. Should be fixed now.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6958
+    auto *WidenLoad = new VPWidenMemoryInstructionRecipe(*Load, Addr, Mask);
+    Plan->addVPValue(Load, WidenLoad);
+    return WidenLoad;
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1167
+class VPWidenMemoryInstructionRecipe : public VPRecipeBase, public VPValue {
   VPUser User;
 
----------------
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)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1582
 
+  SmallVector<VPValue *, 16> VPValuesToFree;
+
----------------
dmgreen wrote:
> Most variables here have a comment.
I added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84680