[PATCH] D97378: [VPlan] Support to vectorize inner loops with VPlan native path enabled

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1837
+
+  // Only use VPlan native path when given loop is not the innermost loop and
+  // VPlan native path is enabled.
----------------
I'd not specify the conditions when this is set true. I'd say something like `Controls whether the VPlan native path is used or not.`

nit: use docygen comment `///`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4443
   PHINode *P = cast<PHINode>(PN);
-  if (EnableVPlanNativePath) {
-    // Currently we enter here in the VPlan-native path for non-induction
-    // PHIs where all control flow is uniform. We simply widen these PHIs.
+  if (EnableVPlanNativePath && !OrigLoop->isInnermost()) {
+    // We enter here in the VPlan-native path and when the loop is not the
----------------
Kazhuu wrote:
> Kazhuu wrote:
> > a.elovikov wrote:
> > > Kazhuu wrote:
> > > > fhahn wrote:
> > > > > I think we should probably have a `UseVPlanNativePath` variable in `ILV`, which is true if `EnableVPlanNativePath && !OrigLoop->isInnerMost()` and replace all checks of `EnableVPlanNativePath` with checking `UseVPlanNativePath`.
> > > > Added the boolean to `LoopVectorizationCostModel` class instead because the flags were used there as well and to avoid having the same flag in both ILV and `LoopVectorizationCostModel` classes.
> > > For my education, why shouldn't we use VPlan native path for inner loop vectorization? Isn't it a simpler way to make the path stable enough (it's already covered by an options that is disabled by default and is served for development purposes only)? I'd expect inner loop vectorization should be a subset of what it should be able to do.
> > > 
> > > In other words, do we expect it to be a temporary workaround to enable more work on VPlan native path right now, or do I miss some design decision that would explain why this should be a long term fix?
> > > 
> > > To clarify - I'm not going to block the review/request changes to the patch because of this. It is solely for the purpose of me getting a better understanding of the ongoing VPlan development and future plans.
> > I wanted to fix this because of the crashes it causes which in my opinion are not the good thing have. To be honest I'm not aware of the long term plan that well either. Maybe @Florian or someone who knows the plan better can shed some light on this?
> Oh no sorry. I meant @fhahn :D
> In other words, do we expect it to be a temporary workaround to enable more work on VPlan native path right now, or do I miss some design decision that would explain why this should be a long term fix?

Unfortunately the VPlan native path is extremely constrained at the moment, exclusively relying on user annotation for legality checks.  Most recent work has been in the other direction, VPlanization of the inner loop vectorizer, so in a way the term 'VPlan native path' is becoming more inaccurate and in the medium term the 'plan native path' will be more like 'VPlan outer loop vectorization'.

In terms of user experience, I think the behavior of the patch is much more user-friendly: currently the VPlan native path also runs on inner loops without requiring any pragmas, causing crashes as outlined in the bug-report. With the current patch, inner loops should go through the existing inner loop vectorizer and only outer loops explicitly marked for vectorization will go through the VPlan native path. This should make it more use-able for people who want to experiment with it on larger code-bases, where they only want to use it for certain loops.

I don't think we are losing much, as we are *very* far off from the VPlan native path being ready and as I said work is underway VPlanizing the inner loop vectorizer towards unifying the paths. This also means the focus of the VPlan native path can remain brining up pieces specifically needed for outer loops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97378