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

[Gil Rapaport via Phabricator via llvm-commits]

gilr added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:433
+  /// and DST.
+  VectorParts createEdgeMask(BasicBlock *Src, BasicBlock *Dst);
+
----------------
rengolin wrote:
> 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.
Under VPlan, ILV is transitioning into a provider of code-gen services to the Planner and Recipes. It seems reasonable to separate the services it provides, which is what it does, from its internal utilities.

While the public/friend design discussion is valid, ILV currently has a public section and OTOH no friends so befriending these two Recipes with it would make an exception in the current 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:
----------------
rengolin wrote:
> nit: I'd mention "elsewhere" is its own recipe.
Ok, will merge this with the default case.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7966
+  void execute(VPTransformState &State) override {
+    State.ILV->vectorizeMemoryInstruction(&Instr);
+  }
----------------
rengolin wrote:
> 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.
Right. In general, ILV's code-gen methods should be refactored such that the decision-taking logic is moved to the Planner and the IR generation is moved to the relevant Recipe's execute() method (as applied to createBlockInMask() & createEdgeMask() in [[ https://reviews.llvm.org/D38676 | D38676 ]]).
Agreed, not for this patch.


https://reviews.llvm.org/D39068