Page MenuHomePhabricator

[LV] Mark first-order recurrences as allowed exit
ClosedPublic

Authored by Ayal on Apr 15 2020, 8:11 AM.

Details

Summary

First-order recurrences require special treatment when they are live-out; such treatment is provided by fixFirstOrderRecurrence(), so they should be included in AllowedExit set. (Should probably have been included originally in D16197.)

Fixes PR45526: AllowedExit set is used by prepareToFoldTailByMasking() to check whether the treatment for live-outs also holds when folding the tail, which is currently the case for reductions but not (yet) for first-order recurrences or other values.

Diff Detail

Event Timeline

Ayal created this revision.Apr 15 2020, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 8:11 AM

I'm not sure how correct this fix is but I verified that it fixes my original problem.

fhahn accepted this revision.Apr 16 2020, 7:26 AM

LGTM, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
671

nit: it seems a bit counter-intuitive to add to something called AllowedExit to reject loops with users of Phi outside the loop. There might be potential for re-naming/refactoring to make it clearer in future. In the meantime it might make sense to add a comment.

llvm/test/Transforms/LoopVectorize/optsize.ll
129

This seems like a very subtle way to check vectorization is not happening. Given that the function is small, it might be better to check the IR for the whole function (or just the control-flow)

This revision is now accepted and ready to land.Apr 16 2020, 7:26 AM
Ayal marked 2 inline comments as done.Apr 18 2020, 4:54 AM
Ayal added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
671

Not sure what comment may be worth adding where. AllowedExit was introduced originally to record all exits that can be vectorized, and first-order-recurring phi's should have been included in it when support for vectorizing them was first introduced (in D16197, as noted in the summary).
AllowedExit is examined by prepareToFoldTail because only a subset of generally vectorizable exits are also supported when vectorizing with foldTail. Perhaps rename AllowedExit to AllowedGeneralExit ?
Above TODO was added to revisit the use of AllowedExit in general.

llvm/test/Transforms/LoopVectorize/optsize.ll
129

ok, replacing this CHECK-NOT with a positive check that the IR remains intact, which is small enough in this case.
Other tests check non-vectorization by CHECK_NOT's of specific vector types or of vector.body.

Ayal updated this revision to Diff 258511.Apr 18 2020, 4:57 AM

Addressed comment regarding test; rebased.

fhahn accepted this revision.Apr 18 2020, 6:33 AM

LGTM, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
671

Above TODO was added to revisit the use of AllowedExit in general.

Right, I think the existing TODO is fine.

This revision was automatically updated to reflect the committed changes.
Ayal added a comment.Apr 18 2020, 3:34 PM

I'm not sure how correct this fix is but I verified that it fixes my original problem.

Thanks @skatkov; patch committed, PR45526 closed.