[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:
> > > 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D44338