[PATCH] D116928: [LoopVectorize] Support epilogue vectorisation of loops with reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @kmclaughlin thanks for sharing this patch. Overall the approach looks sensible, i.e. updating the VPlan for the epilogue to use a new start value for the `VPReductionPHIRecipe`, and moving the bc.merge.rdx to the correct block. The implementation itself still seems a bit fuzzy, so I've given some suggestions to make the remapping of values/edges more explicit.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1231
+    auto *I = dyn_cast<Instruction>(PN->getIncomingValue(In));
+    if (I->getParent() == LoopScalarPreHeader)
+      return I;
----------------
Instead of inferring this information from the PHI node in the original loop, I'd prefer to record the resume values separately in the ILV and pass the information through the EpilogueLoopVectorizationInfo.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4339
+  if (ResumeInst)
+    ResumePhi = dyn_cast<PHINode>(ResumeInst);
+
----------------
It's not necessarily safe to assume ResumePhi is a resume value just because it is a PHINode, so I'd recommend looking this up in the map that I mentioned earlier.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4350-4351
+  for (unsigned I = 0, E = LoopBypassBlocks.size(); I != E; ++I) {
+    if (ResumeInst && LoopBypassBlocks[I] == ResumeInst->getParent())
+      BCBlockPhi->addIncoming(ResumeVal, LoopBypassBlocks[I]);
+    else if (ResumePhi &&
----------------
This case seems a bit odd, does it matter that ResumeValue is an instruction?

For the preheader after epilogue vectorization, I'd expect there to only be three cases to consider:
1. Original start value (when not having executed any vector code at all).
2. Intermediate reduction value after the main vector body (i.e. no epilogue vectorization because of the iter checks)
3. Intermediate reduction value after the vectorized epilogue. (i.e. the VPReductionPHI's new start value).

If you know that ResumePhi is actually a resume PHI node, you can write the loop as:

  for (auto *Incoming : predecessors(LoopScalarPreHeader)) {
    if (Incoming == LoopMiddleBlock)
      BCBlockPhi->addIncoming(ReducedPartRdx, Incoming);
    else if (ResumePhi &&              // case 2 from above.
                                       // note that ResumePhi is only non-zero for epilogue vectorization.
             llvm::is_contained(ResumePhi->blocks(), Incoming))
      BCBlockPhi->addIncoming(ResumePhi->getIncomingValueForBlock(Incoming),
                              Incoming);
    else
      BCBlockPhi->addIncoming((Value *)ReductionStartValue, Incoming);
  }
  
  // Now, we need to fix the users of the reduction variable
  // inside and outside of the scalar remainder loop.
  
  [...]


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8325
+    SmallVector<BasicBlock *, 8> BlocksToRemove;
+    for (unsigned int In = 0; In < Phi.getNumIncomingValues(); In++) {
+      BasicBlock *InBlock = Phi.getIncomingBlock(In);
----------------
Instead of going through all the basic blocks like this, you can simply remove the incoming values from the `EPI.SCEVSafetyCheck`, `EPI.MemSafetyCheck` and `EPI.EpilogueIterationCountCheck`.

The incoming edge from the middle block can be updated to now come from `VecEpilogueIterationCountCheck`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116928