[PATCH] D92132: [LV] Support widened induction variables in epilogue vectorization.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7883
 
   // Keep track of bypass blocks, as they feed start values to the induction
   // phis in the scalar loop preheader.
----------------
Ayal wrote:
> nit: and reduction?
Added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7908
+    // reduction phis only.
+    if (all_of(Phi->blocks(), [&](BasicBlock *IncB) {
+          return EPI.EpilogueIterationCountCheck != IncB;
----------------
Ayal wrote:
> nit: none_of?
Thanks, I wasn't aware of `none_of`. Updated!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10502
+        // before vectorizing the epilogue loop.
         for (VPRecipeBase &R : Header->phis()) {
           if (auto *ReductionPhi = dyn_cast<VPReductionPHIRecipe>(&R)) {
----------------
Ayal wrote:
> nit: can cast R into VPHeaderPHIRecipe and have it provide SetStartValue() to be used by all below.
Added the helper and use it below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10508
               ReductionPhi->setOperand(0, StartVal);
             }
           }
----------------
Ayal wrote:
> early continue? Or have all cases issue a common R.setOperand(0, StartVal)?
Restructured the code to have a single `setStartValue` below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10522
+            ID = &Ind->getInductionDescriptor();
+          }
+          if (!IndPhi)
----------------
Ayal wrote:
> else it's an FOR and continue - they resume ok?
> 
> Rather than check and continue, better assert (IndPhi && "...") for inductions - whose start/resume value must always be fixed?
The only allowed recipe to skip should be the canonical IV. Added an assert. If we have FORs here it would be a mis-compile.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10527
+              IndPhi, *ID, {EPI.MainLoopIterationCountCheck});
+          R.setOperand(0, BestEpiPlan.getOrAddExternalDef(ResumeVal));
         }
----------------
Ayal wrote:
> Compare with how other "live-ins" are handled by creating empty VPValues registered with VPlan to be filled with Values created during VPlan::prepareToExecute(). Are these live-ins better handled here, inside  LoopVectorizePass::processLoop() which is already overburdened with 400 LOC? Can leave behind a TODO...
Added a todo, thanks! Not sure yet how to best move the code, as it requires access to ILV and prepareToExecute is defined in VPlan.cpp, so doesn't really have access to it at the moment unfortunately.


================
Comment at: llvm/test/Transforms/LoopVectorize/optimal-epilog-vectorization-limitations.ll:89
   ret i32 %i.0.lcssa
 }
----------------
Ayal wrote:
> worth retaining f3 testcase to demonstrate that widened/truncated inductions do get epilog vectorized, or becomes redundant?
Sounds good, I moved it to `optimal-epilog-vectorization.ll`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92132