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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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
----------------



================
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;
----------------
/// or drop them in case of conditional assumes.


================
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");
----------------
nit (independent of this patch): pass MaskedOp instead of filling TmpMaskedOp and copying it over?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:492
+      }
       if (R.mayHaveSideEffects() || any_of(R.definedValues(), [](VPValue *V) {
             return V->getNumUsers() > 0;
----------------
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();
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157034