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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Nice clean-up!

(Inspired by "All You Need Is Superword-Level Parallelism: Systematic Control-Flow Vectorization with SLP", Yishen Chen
, Charith Mendis and Saman Amarasinghe, PLDI'22.)



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2838
+    if (Cloned->getNumOperands() == I.index())
+      break;
     auto InputInstance = Instance;
----------------
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?


================
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);
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8944
 
+  VPlanTransforms::addReplicateRegions(*Plan, RecipeBuilder);
+
----------------
comment?

Is this the best position - must be placed before sinkScalarOperands()/mergeReplicateRegionsIntoSuccessors(), possibly right before them?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:688
+    VPBasicBlock *CurrentBlock = RepR->getParent();
+    VPBasicBlock *SplitBlock = CurrentBlock->splitAt(RepR->getIterator());
+
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:83
 
+  static void addReplicateRegions(VPlan &Plan, VPRecipeBuilder &Builder);
+
----------------
Documentation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143865