[PATCH] D44338: [LV][VPlan] Build plain CFG with simple recipes for outer loops.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7704
+    if (VPlanBuildStressTest)
+      return {UserVF, 0};
+
----------------
fhahn wrote:
> dcaballe wrote:
> > fhahn wrote:
> > > dcaballe wrote:
> > > > fhahn wrote:
> > > > > Could we here just return NoVectorization and get rid of the additional check in LoopVectorizePass::processLoop? Also, could we move the setting of UserVF in `plan` too? Otherwise it seems harder to keep track of what's going on and we set UserVF even for inner loops.
> > > > > 
> > > > > Also, I think ideally we would only bail out if the outer loop is not supported, but achieving that seems more trouble than it's worth.
> > > > > Could we here just return NoVectorization and get rid of the additional check in LoopVectorizePass::processLoop?
> > > > 
> > > > Ok. It sounds good to me, at least for now. Hopefully we won't introduce anything after `plan` that we must skip in stress testing mode. Thanks!
> > > > 
> > > > > Also, could we move the setting of UserVF in plan too? Otherwise it seems harder to keep track of what's going on and we set UserVF even for inner loops.
> > > > 
> > > > Could you please elaborate a bit more? I'm not sure I understand what you mean.
> > > > 
> > > > > Also, I think ideally we would only bail out if the outer loop is not supported, but achieving that seems more trouble than it's worth.
> > > > 
> > > > We can think about it in the future. We would need legality analysis for outer loops if we want to generate code.
> > > > Ok. It sounds good to me, at least for now. Hopefully we won't introduce anything after plan that we must skip in stress testing mode. Thanks!
> > > 
> > > If we return NoVectorization, I would assume we would not use the generated plans after `plan`, as we decided to skip vectorization?
> > > 
> > > > Could you please elaborate a bit more? I'm not sure I understand what you mean.
> > > 
> > > I meant moving the code to set UserVF  = 4 if VPlanBuildStressTest into this function. That would reduce the number of functions where we have to handle VPlanBuildStressTest, which IMO makes it easier to see what's going on.
> > > 
> > > If we return NoVectorization, I would assume we would not use the generated plans after plan, as we decided to skip vectorization?
> > 
> > Yes, it was just a thought. For example, stress testing will be skipping some checks (only `isExplicitVecOuterLoop`, for now). If we have code after `plan` expecting a loop compliant with those checks, we could have problems. But, again, I agree on the change. We can deal with that if it happens.
> > 
> > >I meant moving the code to set UserVF  = 4 if VPlanBuildStressTest into this function. That would reduce the number of functions where we have to handle VPlanBuildStressTest, which IMO makes it easier to see what's going on.
> > 
> > Thanks, got it! I wonder if this would be problematic. If we moved this change into this function and we used UserVF after `plan` (likely to happen, look at uses of line 8794), we would be using inconsistent UserVF values. Maybe it's better to keep it here?
> But as it is currently, the UserVF set in the outer loop code path limited to that block. And I cannot think of any case where using UserVF set for VPlanBuildStressTest will useful anywhere else for now, especially the inner loop code path. My understanding was that we have to bail after planning anyways for VPlanBuildStressTest. At least as everything is now.
Ok, fair enough. I'll move it into the function, at least, for now.
Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D44338