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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:433
+  /// and DST.
+  VectorParts createEdgeMask(BasicBlock *Src, BasicBlock *Dst);
+
----------------
gilr wrote:
> 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.
I agree on two points:

1. The discussion between public/friend (something else) doesn't need to be done in this patch, but it does need to happen.
2. It's good to separate provider/user, but the way we're going, the public methods in ILV could unintentionally create coupling between recipes. (true, friend/public won't change solve that problem).

I worry more about (2) than (1), and that's why I think this discussion can happen outside of this patch.

Once ILV becomes an empty shell of utility methods, we should consider moving all services to LoopUtils.


https://reviews.llvm.org/D39068