[PATCH] D112552: [LoopVectorize] When tail-folding, don't always predicate uniform loads
Sander de Smalen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 5 04:36:26 PDT 2021
sdesmalen added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9097
}
+
LLVM_DEBUG(dbgs() << "LV: Scalarizing and predicating:" << *I << "\n");
----------------
nit: unrelated whitespace change?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9070
+ if (IsUniform && isa<LoadInst>(I) &&
+ !Legal->blockNeedsPredication(I->getParent()) && Range.Start.isVector()) {
+ assert(CM.foldTailByMasking() &&
----------------
david-arm wrote:
> fhahn wrote:
> > The problem with the workaround is that `IsPredicated` is now inaccurate for the recipe I think.
> >
> > It looks like the cost-model already accurately estimates the cost as not requiring predication. So perhaps it would be better to not claim the instruction requires predication in the first place in `LoopVectorizationCostModel::isPredicated`?
> Hi @fhahn, are you suggesting that `IsPredicated` is being set incorrectly above? If I understand correctly, I believe you're suggesting the answer would be to add more logic to `isPredicatedInst`? i.e. something like:
>
> bool isPredicatedInst(Instruction *I, bool IsUniform) {
> if (IsUniform && isa<LoadInst>(I) &&
> !Legal->blockNeedsPredication(I->getParent()))
> return false;
>
> if (!blockNeedsPredication(I->getParent()))
> ...
> }
>
> That feels a bit messy. Would it be better to simply adjust the IsPredicated boolean value instead in this function?
Hi @david-arm, I think that could just be:
// Returns true if \p I is an instruction that will be predicated either
// through scalar predication or masked load/store or masked gather/scatter.
// Superset of instructions that return true for isScalarWithPredication.
bool isPredicatedInst(Instruction *I, bool IsUniform = false) {
if (!blockNeedsPredication(I->getParent()))
return false;
// Uniform loads don't require predication.
if (isa<LoadInst>(I) && IsUniform)
return false;
// Loads and stores that need some form of masked operation are predicated
// instructions.
if (isa<LoadInst>(I) || isa<StoreInst>(I))
return Legal->isMaskRequired(I);
return isScalarWithPredication(I);
}
Which seems like a sensible change to me and isn't really that messy?
Looking at the uses of that function, should the then-block of `if (isPredicatedInst(I))` in `getMemInstScalarizationCost` have an assert that the the value is not uniform?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112552/new/
https://reviews.llvm.org/D112552
More information about the llvm-commits
mailing list