This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port LCSSA to the new PM
ClosedPublic

Authored by eraman on Jun 7 2016, 11:58 AM.

Details

Summary

The new pass does not preserve the same set as the legacy pass and I've added FIXMEs to indicate what is missing.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 59924.Jun 7 2016, 11:58 AM
eraman retitled this revision from to [PM] Port LCSSA to the new PM.
eraman updated this object.
eraman added a reviewer: chandlerc.
eraman added a subscriber: llvm-commits.
davide added a subscriber: davide.Jun 7 2016, 12:02 PM
davide added inline comments.
lib/Transforms/Utils/LCSSA.cpp
30 ↗(On Diff #59924)

mhm,did you forget LCSSA.h maybe? This doesn't build.

eraman updated this revision to Diff 59926.Jun 7 2016, 12:10 PM

Update the patch to include missing header file

Some comments (waiting for the header).

lib/Transforms/Utils/LCSSA.cpp
342 ↗(On Diff #59924)

else after return

348 ↗(On Diff #59924)

I'd put the comment just before the declaration of PA

test/Transforms/LCSSA/2006-06-12-MultipleExitsSameBlock.ll
5 ↗(On Diff #59924)

This should really use FileCheck and not grep. Feel free to commit the change to the existing code separately.

test/Transforms/LCSSA/basictest.ll
3 ↗(On Diff #59924)

Ditto.

eraman updated this revision to Diff 59959.Jun 7 2016, 3:12 PM
eraman marked 3 inline comments as done.

Address Davide's comments and rebase.

davide added inline comments.Jun 8 2016, 6:56 AM
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

This is a sore point. I think mustPreserveAnalysisID(LCSSAID) should become an assert in the new PM which verifies that the IR mutation didn't destroy LCSSA.
In other words, mustPreserveAnalysisID(LCSSAID) might need to go away.

eraman marked an inline comment as done.Jun 8 2016, 2:53 PM
eraman added inline comments.
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

Could you elaborate on this a bit more? The current state is that loop transformation passes call mustPreserveAnalysisID(LCSSAID) to determine if LCSSA is available and ensures that the transformation preserves LCSSA by calling addPreserved. How will this look in the new PM?

Incidentally, LoopSimplify (and a few others) do not actually call addPreservedID(LCSSAID). Isn't this a bug?

test/Transforms/LCSSA/2006-06-12-MultipleExitsSameBlock.ll
5–9 ↗(On Diff #59926)

I committed r272065 that made the original tests use FileCheck

mzolotukhin added inline comments.Jun 8 2016, 3:54 PM
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

The fact that LoopSimplify doesn't do addPreservedID(LCSSAID) looks like a bug to me (which I'm trying to address in D21112 btw). What are the other passes that do the same?

davide added inline comments.Jun 8 2016, 4:38 PM
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

Sure.

  1. My understanding is that LCSSA isn't really referential transparent/pure (in an haskell'y meaning of the word), and it's fairly easy to forge cases where it mutates the IR. Therefore, in the new (caching-based) PM, it's very hard to model the caching of LCSSA.

What I think (and what I gathered from Chandler) is that in this case instead of caching we should just add a verifier pass which asserts LCSSA is maintained.

  1. I think LoopSimplify should maintain LCSSA, and probably other passes in the LoopPM (which maybe don't today)
davide added inline comments.Jun 8 2016, 4:49 PM
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

To be clear: when I say LCSSA is not pure I mean the LCSSA pass in LLVM, not the canonical form per-se.

Thanks!

chandlerc accepted this revision.Jun 8 2016, 6:18 PM
chandlerc edited edge metadata.

I'm good with the patch with the FIXME below removed, but you should circle back with Davide before landing.

lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

Sorry for not chiming in earlier, just now catching up on email.

I feel like some of this discussion would be helped by re-reading the design discussion around the new pass manager. There is a fundamental difference regarding the new pass manager and the old pass manager when it comes to LCSSA and LoopSimplify. I'll try to summarize that here:

The old pass manager uses the analysis preserved / requires machinery to *automatically* schedule the *transformation* passes LCSSA and LoopSimplify. This is incredibly magical and brittle. It breaks constantly and has proven nearly impossible to support.

The new pass manager consequentially has *zero support for this*. The intent is that LCSSA and LoopSimplify will be canonicalizing transformation passes for loops. They'll be scheduled explicitly, and loop passes should detect and gracefully degrade behavior when loops are not simplified or not in LCSSA form. Transformations should still work to not *break* LCSSA because that is the desired form for loops.

I understand that this result will be painful, but I think it is by far the less painful alternative to the current design. We spend much too much time fixing crashers that are hard to even test for because of the requires / preserves nonsense.

I think the only realistic way to make LCSSA *required* (which seems desirable considering the cost of checking for it) is to make it *verifier required* which requires making Loops part of the IR the way Dominators are, etc, etc, etc. That's a huge project, and I don't think we want to try to do that right now.

So ultimately, I don't think there is any FIXME needed here.

This revision is now accepted and ready to land.Jun 8 2016, 6:18 PM
davide added a comment.Jun 8 2016, 7:11 PM

LGTM if Chandler is happy with it.

eraman marked an inline comment as done.Jun 9 2016, 11:46 AM
eraman added inline comments.
lib/Transforms/Utils/LCSSA.cpp
345 ↗(On Diff #59959)

LoopInterchange and PPCLoopPreIncPrep.cpp call mustPreserveAnalysisID(LCSSAID) but do not preserve LCSSA.

345 ↗(On Diff #59959)

Thanks for the summary. I have a better understanding now. Will submit with the FIXMEs removed.

This revision was automatically updated to reflect the committed changes.