Page MenuHomePhabricator

[SCEV] Don't bother preserving LCSSA in SCEV
AbandonedPublic

Authored by mkazantsev on Feb 1 2019, 12:00 AM.

Details

Summary

Preserving LCSSA of IR should be responsibility of transform passes. We should not try to
keep SCEV in LCSSA form because because of this we can lose some transform opportunitites.

Diff Detail

Event Timeline

mkazantsev created this revision.Feb 1 2019, 12:00 AM
mkazantsev marked an inline comment as done.Feb 1 2019, 12:10 AM
mkazantsev added inline comments.
lib/Analysis/ScalarEvolution.cpp
5346–5347

Basically, I guess that we could just have deleted this check for same effect. Is there a reason to keep it? What's bad about breaking LCSSA in SCEV?

mkazantsev updated this revision to Diff 184693.Feb 1 2019, 1:36 AM
mkazantsev retitled this revision from [SCEV] Optimize getting SCEV of one-input Phis to [SCEV] Don't bother preserving LCSSA in SCEV.
mkazantsev edited the summary of this revision. (Show Details)
fhahn accepted this revision.Feb 1 2019, 6:09 AM
fhahn added a subscriber: fhahn.

LCSSA form is a property of the IR. As we do not change the IR here, there is no way we could destroy the LCSSA form of the underlying IR. If transformations use the extra info to destroy LCSSA form, they should be fixed.

LGTM, but please wait a bit with committing, in case the other reviewers have additional comments or I missed something.

lib/Analysis/ScalarEvolution.cpp
5342

This comment is obsolete now.

test/Transforms/LoopStrengthReduce/funclet.ll
48

not really related to this patch, but it is not really clear what gets checked here and elsewhere. I think it would be great if we could add a clear test case where this change is beneficial.

This revision is now accepted and ready to land.Feb 1 2019, 6:09 AM
sanjoy accepted this revision.Feb 1 2019, 9:06 PM

I'm okay with the direction, but I'm also worried that this breaks some subtle invariant between SCEV and SCEVExpander. Have you tried bootstrapping clang with this change to see if that shakes out any issues?

I'll run some extensive testing to see if it's breaking something. If yes, we have bugs to fix ahead.

mkazantsev planned changes to this revision.Feb 3 2019, 11:41 PM

My fuzz testing has detected LCSSA breaches. My conviction is that it's a right way to go, and the failures should be fixed where they are dont, but we can't merge it before the bugs are fixed there.

dmgreen added a subscriber: dmgreen.Feb 4 2019, 4:29 AM
mkazantsev abandoned this revision.Feb 13 2019, 2:25 AM

Looks like there is a lot of places in code that expect that... :(

Looks like there is a lot of places in code that expect that... :(

:(