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

[Sander de Smalen via Phabricator via llvm-commits]

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