[PATCH] D157034: [VPlan] Model masked assumes as replicate recipes, drop them (NFCI).
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 4 14:22:36 PDT 2023
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
More information about the llvm-commits
mailing list