[PATCH] D157034: [VPlan] Model masked assumes as replicate recipes, drop them (NFCI).

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:448
   /// \p MaskedOp is a list of instructions that have to be transformed into
   /// calls to the appropriate masked intrinsic when the loop is vectorized.
+  bool
----------------
Ayal wrote:
> 
updated, thanks!


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:534
   /// While vectorizing these instructions we have to generate a
   /// call to the appropriate masked intrinsic
   SmallPtrSet<const Instruction *, 8> MaskedOp;
----------------
Ayal wrote:
> /// or drop them in case of conditional assumes.
Updated thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1560
   for (BasicBlock *BB : TheLoop->blocks()) {
-    if (!blockCanBePredicated(BB, SafePointers, TmpMaskedOp,
-                              TmpConditionalAssumes)) {
+    if (!blockCanBePredicated(BB, SafePointers, TmpMaskedOp)) {
       LLVM_DEBUG(dbgs() << "LV: Cannot fold tail by masking as requested.\n");
----------------
Ayal wrote:
> nit (independent of this patch): pass MaskedOp instead of filling TmpMaskedOp and copying it over?
Hmm, this might be intentional, to avoid populating `MaskedOp` partially if any block cannot be predicated.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8930
       // built for them.
-      if (isa<BranchInst>(Instr) || DeadInstructions.count(Instr))
+      if (isa<BranchInst>(Instr))
         continue;
----------------
Ayal wrote:
> nit: if Instr is a branch there's not much left to continue.
Updated to drop the terminator when iterating over the block.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:492
+      }
       if (R.mayHaveSideEffects() || any_of(R.definedValues(), [](VPValue *V) {
             return V->getNumUsers() > 0;
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > (Other, unpredicated) Assumes are not removed because they are considered to have side effects or have droppable users?
> > Presumably they are retained as they may add info helping later optimizations.
> > (Other, unpredicated) Assumes are not removed because they are considered to have side effects or have droppable users?
> 
> It is the former - assumes are considered to have side effects (return void, have no users).
> Would it be better to exclude conditional assumes from the mayHaveSideEffect waivor, e.g., something like:
> 
> 
> ```
>       // A user keeps R alive:
>       if (any_of(R.definedValues(), [](VPValue *V) { return V->getNumUsers(); }))
>         continue;
> 
>       // Having side effects keeps R alive, but do remove conditional assume
>       // instructions as their conditions may be flattened.
>       auto *RepR = dyn_cast<VPReplicateRecipe>(&R);
>       bool IsConditionalAssume = RepR && RepR->isPredicated() &&
>           match(RepR->getUnderlyingInstr(), m_Intrinsic<Intrinsic::assume>());
>       if (R.mayHaveSideEffects() && !IsConditionalAssume)
>         continue;
> 
>       R.eraseFromParent();
> ```
> ?
Updated, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157034