[PATCH] D39068: [LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

The patch is much easier to review now, mostly code movement. Thanks!

Overall, it looks good and is on par with what I was expecting to see when moving internal recipes to VPlans.

I strongly believe we shouldn't be making methods public, even if just while moving to VPlans.

Given that we are now asserting when seeing memory instructions in the generic widener (a thing that we may drop as soon as we move all recipes out, and just make it a generic assert for all non handled instruction types), I believe it should be fine to move the code inside the plans themselves, instead of creating the call backs.

Who else calls `vectorizeMemoryInstruction` or the others? It would be good to know and make sure they don't (or why they do and create tasks to clear that up).

cheers,
--renato



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:433
+  /// and DST.
+  VectorParts createEdgeMask(BasicBlock *Src, BasicBlock *Dst);
+
----------------
IMHO, we shouldn't make methods public just because we need them from outside. Protection levels should be selected based on what they do, not who may use them.

To me, the need to move them public is indication that the design needs more thought. I'm ok with the thought and implementation being in another patch, but making them public here may encourage bad practices elsewhere.

If anything else fails, we probably should make the recipes `friend` instead, with a very big warning comment that we need to think more about the overall design.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4738
   case Instruction::Load:
-    vectorizeMemoryInstruction(&I);
-    break;
+    llvm_unreachable("Memory instructions are widened elsewhere");
   case Instruction::ZExt:
----------------
nit: I'd mention "elsewhere" is its own recipe.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7966
+  void execute(VPTransformState &State) override {
+    State.ILV->vectorizeMemoryInstruction(&Instr);
+  }
----------------
Maybe not for this patch, but I think the logic should be moved inside this class, and not be a call back to the ILV.


https://reviews.llvm.org/D39068