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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5307
+    Instruction *I, bool VFIterationIsPredicated) const {
+  if (!Legal->requiresPredicatedWidening(I, VFIterationIsPredicated))
     return false;
----------------
sdesmalen wrote:
> 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?
Thanks, that definitely helped, and I now see where I went wrong.

These explanations are better (in my opinion) than the current comments for these functions. If you can update those comments that would be good.

I am now going to read the patch again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102437