[PATCH] D46827: [LV] Add VPInstruction to VPRecipe transformation.

Diego Caballero via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 14 21:51:05 PDT 2018


dcaballe added a comment.

Hi Florian. Thanks for the patch!

> This patch just moves things around and makes things clearer, but it modifies
> the inner loop vectorizer. I think we should try to use as many parts of VPlan
> for inner loop vectorization as well, to make sure we get as much
> testing early on. But I am not entirely sure if it would be better to have
> a completely separate inner loop vectorization path in the VPlan native path, to
> eliminate the risk of breaking things in the inner loop vectorizers.
> What do you think?

I agree with you but I think it's too soon to enable some of this code in the inner loop vectorizer. I would wait at least until we have codegen support for outer loops so that we can do a more proper testing before we use it for production. In addition, another big concern is that introducing this code in the inner loop vectorizer too soon will limit the current development flexibility that we have in the VPlan native path. For those reasons, I would prefer this patch to just do the VPInstruction2VPRecipe transformation for the VPlan native path and leave the inner loop vectorizer changes for later. Does it make sense to you?



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:363
+
+  VPlanPtr transformVPInstructionsToVPRecipies(VPlanPtr &OriginalPlan,
+                                               VFRange &Range);
----------------
I think we should try to find a better place for this. The planner shouldn't be responsible for VPlan-to-VPlan transformations but just for doing the planning (orchestrating them). Otherwise, it will turn into a big brown bag of unrelated stuff.

Since this is a small and temporal thing and it's kind of related to code generation, maybe we could include it in ILV class as a "prepare" CG step?

Another option would be to create a specific class for it. We could rename VPlanHCFGBuilder.h/.cpp -> VPlanHCFGTransforms.h/.cpp and place there all the "small" VPlan-to-VPlan transformations.


https://reviews.llvm.org/D46827





More information about the llvm-commits mailing list