[PATCH] D112552: [LoopVectorize] When tail-folding, don't always predicate uniform loads

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9070
+  if (IsUniform && isa<LoadInst>(I) &&
+      !Legal->blockNeedsPredication(I->getParent()) && Range.Start.isVector()) {
+    assert(CM.foldTailByMasking() &&
----------------
sdesmalen wrote:
> 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?
> 
> 
> That feels a bit messy. Would it be better to simply adjust the IsPredicated boolean value instead in this function?

Why do you think this would be messy? If we treat those loads as unpredicted during codegen, claiming they will be predicated earlier seems inconsistent and may lead to surprises down the road for cases they are out-of-sync.


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

https://reviews.llvm.org/D112552