[PATCH] D102437: [LV] NFC: Decouple foldTailByMasking from isScalarWithPredication.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1343
+
+bool LoopVectorizationLegality::requiresPredicatedWidening(
+    Instruction *I, bool VFIterationIsPredicated) const {
----------------
SjoerdMeijer wrote:
> I think this is related to the legal vs. cost-model discussion. I agree that it is probably more a cost-model thing. Does that mean this needs to live in CostModel, where it used to live?
Okay, if you feel the same way, I'll just move it to the CostModel!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1354
+  case Instruction::Store:
+    // Some instructions can be speculated, even when predication is used
+    // for the block.
----------------
SjoerdMeijer wrote:
> Just wanted to check what we mean by speculation here (or why it is relevant here)?
The reason I initially added the comment was because I would have expected `return true;` (given that the block needs predication or the VF Iteration is predicated), but it seems LVL uses a more detailed analysis to check whether the operation can be speculatively executed, reflected in `isMaskRequired(I)`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5307
+    Instruction *I, bool VFIterationIsPredicated) const {
+  if (!Legal->requiresPredicatedWidening(I, VFIterationIsPredicated))
     return false;
----------------
SjoerdMeijer wrote:
> I am confused here about names or some negations in logic, but probably it is names.
> 
> Here, if we "do not require predicated widening", we return false for `isScalarWithPredication`, whereas I probably was expecting true. Ignoring some edge cases, `requiresPredicatedWidening` returns true for most cases because `all operations can be widened safely without predication`. So probably I am confused that we are talking about widening and isScalar at the same time, or I am missing something else.
> `requiresPredicatedWidening` returns true for most cases because `all operations can be widened safely without predication`
`requiresPredicatedWidening` returns //false// for most cases, because most operations can be widened safely without requiring predication.

I've tried to split up two questions:
`requiresPredicatedWidening`: Given the use of predication in the loop (either from tail folding, or from the operation being under an actual if-then condition), does widening the operation //require// predication, or conversely: can the operation speculatively execute all lanes followed by a `select` on the output. If the answer to the latter question is 'no' (like the case for DIV), then the instruction requires a predicated vector operation (<=> predicated widening).
`isScalarWithPredication`: Given that the operation needs predicated widening, can we actually widen it, or do we need to scalarise it? If it doesn't require predicated widening, we know it also doesn't need to be scalarised (hence the condition to `return false` here)

Does that help?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102437