[PATCH] D143865: [VPlan] Add predicate to VPReplicateRecipe, expand region later.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2838
+    if (Cloned->getNumOperands() == I.index())
+      break;
     auto InputInstance = Instance;
----------------
Ayal wrote:
> Could the last redundant operand of RepRecipe be removed once it is no longer needed (after wrapping it inside a replicate region) instead of silently ignoring it here?
Updated the code to remove the mask, by creating a new VPReplicateRecipe without the mask.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8526
+    // if-then construct to prevent side-effects.
+    // Generate recipes to compute the block mask for this region.
+    VPValue *BlockInMask = createBlockInMask(I->getParent(), Plan);
----------------
Ayal wrote:
> Worth clarifying that these instructions are first represented as a replicate recipe with a mask (facilitating their motion etc.) and are wrapping within the if-then masked region later?
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8944
 
+  VPlanTransforms::addReplicateRegions(*Plan, RecipeBuilder);
+
----------------
Ayal wrote:
> comment?
> 
> Is this the best position - must be placed before sinkScalarOperands()/mergeReplicateRegionsIntoSuccessors(), possibly right before them?
Added a comment and moved just before sinkScalarOperands/region merging.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:622
     // sinking the containing replicate region.
     if (isa<VPPredInstPHIRecipe>(SinkCandidate) || SinkCandidate == FOR)
       continue;
----------------
Ayal wrote:
> Is this comment and code dealing with PredInstPHI still needed here?
Not any longer, removed!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:688
+    VPBasicBlock *CurrentBlock = RepR->getParent();
+    VPBasicBlock *SplitBlock = CurrentBlock->splitAt(RepR->getIterator());
+
----------------
Ayal wrote:
> Future thought: would it be better to handle intervals of RepR's that share the same mask together here, placing them in a common region, rather than fusing regions later?
that would be a good follow-up change; general merging & sinking will probably still be useful, as sinking can enable new merging opportunities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143865