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

[Mauri Mustonen via Phabricator via llvm-commits]

Kazhuu marked 2 inline comments as done.
Kazhuu added inline comments.


================
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
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5184
     if (isa<LoadInst>(I) && Legal->isUniformMemOp(*I)) {
-      assert(WideningDecision == CM_Scalarize);
+      assert(WideningDecision == CM_Scalarize ||
+             WideningDecision == CM_GatherScatter);
----------------
Kazhuu wrote:
> fhahn wrote:
> > Why is this needed? If we handle inner loops with the regular ILV even in the VPlanNativePath, the assert should not change I think. Is it possible that we need to update other parts to set the right decision for ILV in VPlanNativePath?
> I needed to add this when inner loop contained load instruction that is uniform on all iterations . This load is also in the test case I added in the loop body:
> ```
> %d = load float, float* %arrayidx5.i, align 4
> ```
> I guess technically LICM should lift this load up from the loop but I experienced this error when using PoCL with CPU backend and polybench OpenCL testbech using normal -O3 pass pipeline. I guess I could post it as a separate issue as well if needed. The assert line reported to fail was this:
> ```
> opt: /home/kazooie/extra/programming/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5184: llvm::LoopVectorizationCostModel::collectLoopUniforms(llvm::ElementCount)::<lambda(llvm::Instruction*, llvm::ElementCount)>: Assertion `WideningDecision == CM_Scalarize' failed.
> PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
> 
> ```
> And to my knowledge where scatter/gather widening decision is coming from is form the fact that VPlan doesn't support recognizing uniform memory strides, so it is using scatter/gather instead of loads.
Yeah you were right this was unnecessary indeed when the VPlan flag usage is fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97378