Page MenuHomePhabricator

[LV] Fix miscompiles by adding non-header PHI nodes to AllowedExit

Authored by bjope on Sep 2 2019, 5:26 AM.



Fold-tail currently supports reduction last-vector-value live-out's,
but has yet to support last-scalar-value live-outs, including
non-header phi's. As it relies on AllowedExit in order to detect
them and bail out we need to add the non-header PHI nodes to
AllowedExit, otherwise we end up with miscompiles.


Diff Detail


Event Timeline

bjope created this revision.Sep 2 2019, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 5:26 AM
fhahn added a comment.Sep 2 2019, 5:31 AM

Please update the comment about AllowedExit in LoopVectorizationLegality. Currently it only states that it holds reduction and induction vars, but it really contains any variable that can be accessed outside the loop.

bjope updated this revision to Diff 218357.Sep 2 2019, 6:48 AM

Updated description for AllowedExit.

fhahn accepted this revision.Sep 2 2019, 12:14 PM

LGTM, thanks. Please wait with committing a day or so, in case Ayal has additional comments.

This revision is now accepted and ready to land.Sep 2 2019, 12:14 PM
Ayal accepted this revision.Sep 2 2019, 1:55 PM
Ayal added a subscriber: anna.

This LGTM too; added a couple of minor comments about comments.

449 ↗(On Diff #218357)

This BTW also includes non-phi instructions, as of rL340278 thanks to @anna. As indicated in a TODO below, perhaps it's better to specify what's not included. For now, perhaps it's better to simply avoid specifying what's included.

779 ↗(On Diff #218357)

While we're at it, the above comment deserves updating as well..

785 ↗(On Diff #218357)

(BTW, this SCEV-predicates condition, related to PR33706, should apply only to IV header-phi's and related instructions. But it's conservative; relaxing it calls for a separate optimization fix.)

36 ↗(On Diff #218357)

Please retain the indication that these tests refer to "pr43166". (Such test files are often named pr43166.ll).

This revision was automatically updated to reflect the committed changes.
bjope marked 3 inline comments as done.Sep 3 2019, 2:46 AM

Thanks for all the feedback. Landed as rL370721.

779 ↗(On Diff #218357)

Not sure in what way it should be updated (but it seemed unrelated to the fix so I skipped this).