This is an archive of the discontinued LLVM Phabricator instance.

[LoopInterchange] Fix handling of LCSSA nodes defined in headers and latches.
ClosedPublic

Authored by fhahn on May 5 2019, 2:35 PM.

Details

Summary

The code to preserve LCSSA PHIs currently only properly supports
reduction PHIs and PHIs for values defined outside the latches.

This patch improves the LCSSA PHI handling to cover PHIs for values
defined in the latches.

Fixes PR41725.

Event Timeline

fhahn created this revision.May 5 2019, 2:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2019, 2:35 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn updated this revision to Diff 198204.May 5 2019, 2:54 PM

Add test case without loop exit, clang-format-diff

jdoerfert added inline comments.
llvm/lib/Transforms/Scalar/LoopInterchange.cpp
1320

This seems brittle. I don't know all the preconditions here but I imagine various situations where you might end up with multiple uses for escaping LCSSA phi nodes.

One might be two values escape which happen to be equal, in the inner exit block some optimization merges the two LCSSA phi nodes, in the outer that doesn't happen. Now we have one PHI in the inner exit and two uses in the outer exists. Does this make sense (without asserting we have a transformation like this right now)?

1324

Isn't this a job for P.replaceAllUsesWith(...) ?

fhahn updated this revision to Diff 198249.May 6 2019, 4:53 AM

Remove restriction for single use PHIs. We can replace such PHIs with their incoming
value, iff all uses are in the loop exit block or in the outer loop header, if
the incoming value is defined in the inner loop header (reduction phis).

fhahn marked 2 inline comments as done.May 6 2019, 4:58 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/LoopInterchange.cpp
1320

Yeah, it could theoretically happen (but I think it is unlikely in practice, as the lcssa PHIs would be trivially equivalent). The single use is actually not important. It is important where those uses actually are. The legality checks should ensure that. I updated the assertion.

This revision is now accepted and ready to land.May 14 2019, 5:45 AM
This revision was automatically updated to reflect the committed changes.