Page MenuHomePhabricator

[LV] Pick correct BB as insert point when fixing PHI for FORs.
ClosedPublic

Authored by fhahn on Dec 5 2019, 9:40 AM.

Details

Summary

Currently we fail to pick the right insertion point when
PreviousLastPart of a first-order-recurrence is a PHI node not in the
LoopVectorBody. This can happen when PreviousLastPart is produce in a
predicated block. In that case, we should pick the insertion point in
the BB the PHI is in.

Fixes PR44020.

Diff Detail

Event Timeline

fhahn created this revision.Dec 5 2019, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2019, 9:40 AM

Build result: pass - 60515 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Ayal accepted this revision.Dec 6 2019, 1:45 AM

Looks good to me with minor optional notes.
Consider adding a target-indepedent test, e.g., augmenting first-order-recurrence.ll; tests can possibly be committed before the fix, demonstrating current behavior and then the change brought by this fix.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3563

(Note that Previous that is loop invariant forms no recurrence ... they best be recognized early (if/why not LICM'd prior to LV), and optimized accordingly. A todo for/following D68831?)

3563–3579

Can refactor to have a single Builder.SetInsertPoint(Point); following the different cases for setting Point.

llvm/test/Transforms/LoopVectorize/SystemZ/predicated-first-order-recurrence.ll
4

State what this test is designed to check?
("In the test case we predicate due to low trip count right? It might be worth calling that out." ;-)

This revision is now accepted and ready to land.Dec 6 2019, 1:45 AM
Ayal added inline comments.Dec 6 2019, 1:51 AM
llvm/test/Transforms/LoopVectorize/SystemZ/predicated-first-order-recurrence.ll
4

Mention PR44020

This revision was automatically updated to reflect the committed changes.
fhahn marked 3 inline comments as done.Dec 7 2019, 11:43 AM

Looks good to me with minor optional notes.
Consider adding a target-indepedent test, e.g., augmenting first-order-recurrence.ll; tests can possibly be committed before the fix, demonstrating current behavior and then the change brought by this fix.

I tried, but the test case requires tail folding and it seems to depend on an actual target. We make the decision to tail-fold in computeMaxVF (https://github.com/llvm/llvm-project/blob/c49194969430f0ee817498a7000a979a7a0ded03/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp#L4941) and use the max target VF, not the forced vector width. This seems a bit odd and we might want to change that, to make it easier to test tail-folding without any specific target. What do you think?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3563

I've added a FIXME in the committed version

llvm/test/Transforms/LoopVectorize/SystemZ/predicated-first-order-recurrence.ll
4

Done thanks :)

Ayal added a comment.Dec 7 2019, 1:10 PM

Looks good to me with minor optional notes.
Consider adding a target-indepedent test, e.g., augmenting first-order-recurrence.ll; tests can possibly be committed before the fix, demonstrating current behavior and then the change brought by this fix.

I tried, but the test case requires tail folding and it seems to depend on an actual target. We make the decision to tail-fold in computeMaxVF (https://github.com/llvm/llvm-project/blob/c49194969430f0ee817498a7000a979a7a0ded03/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp#L4941) and use the max target VF, not the forced vector width. This seems a bit odd and we might want to change that, to make it easier to test tail-folding without any specific target. What do you think?

The -prefer-predicate-over-epilog flag should help test tail folding w/o a specific target. Admittedly it is currently used by target-dependent tests only.