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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks!
Adding a couple of final nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8701
   auto *Entry = new VPBasicBlock(Twine(RegionName) + ".entry", BOMRecipe);
   auto *PHIRecipe = Instr->getType()->isVoidTy()
                         ? nullptr
----------------
nit: now that this is a VPlan2VPlan transformation, check if PredRecipe has users instead of Instr's type?

While we're here, could perhaps be simplified into (independent of this patch):

```
VPRecipeBase *PHIRecipe = nullptr;
if (PredRecipe has users outside its block) {
  PHIRecipe = new VPPredInstPHIRecipe(RecipeWithoutMask);
  RecipeWithoutMask->replaceAllUsesWith(PHIRecipe);
  PHIRecipe->setOperand(0, RecipeWithoutMask);
}
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9186
     ShouldSimplify |= VPlanTransforms::mergeBlocksIntoPredecessors(*Plan);
   }
 
----------------
nit: the above creation and iterative optimization of replicate regions could be folded into some `VPlanTransforms::createAndOptimizeReplicateRegions(*Plan, RecipeBuilder);`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:83
 
+  static void addReplicateRegions(VPlan &Plan, VPRecipeBuilder &Builder);
+
----------------
fhahn wrote:
> Ayal wrote:
> > Documentation?
> Should be added, thanks!
nit: perhaps better place addReplicateRegions() above right before sinkScalarOperand() - merge - merge?


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop-cond.ll:411
+; CHECK-NEXT:    [[TMP37:%.*]] = load i32, ptr [[TMP36]], align 4
+; CHECK-NEXT:    [[TMP38:%.*]] = insertelement <4 x i32> [[TMP30]], i32 [[TMP37]], i64 3
 ; CHECK-NEXT:    br label [[PRED_LOAD_CONTINUE6]]
----------------
fhahn wrote:
> Ayal wrote:
> > Two independent replicating regions with a reduce.add between them that were kept separate are now being merged? This patch seems conceptually an NFC, according to its current commit message.
> In this case, all recipes are in the same VPBasicBlock when `adjustRecipesForReductions` is called, which will add the reduction recipes at the end of the block containing the original widen recipes, hence the change.
Wonderful!
Worth clarifying at commit message that this construction simplification may yield better optimized code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143865