[PATCH] D21090: [PM] Port LCSSA to the new PM

[Easwaran Raman via llvm-commits]

eraman marked an inline comment as done.

================
Comment at: lib/Transforms/Utils/LCSSA.cpp:345
@@ +344,3 @@
+  // ported to the new PM and the legacy LoopSimplify pass has a call to
+  // mustPreserveAnalysisID(LCSSAID) in its runOnFunction method. So, for now,
+  // we are not preserving LoopSimplify.
----------------
chandlerc wrote:
> davide wrote:
> > davide wrote:
> > > mzolotukhin wrote:
> > > > eraman wrote:
> > > > > davide wrote:
> > > > > > 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.
> > > > > 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?  
> > > > > 
> > > > > 
> > > > 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?
> > > 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. 
> > > 
> > > 2) I think LoopSimplify should maintain LCSSA, and probably other passes in the LoopPM (which maybe don't today)
> > To be clear: when I say LCSSA is not pure I mean the LCSSA pass in LLVM, not the canonical form per-se.
> > 
> > Thanks!
> 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.
LoopInterchange and  PPCLoopPreIncPrep.cpp call mustPreserveAnalysisID(LCSSAID) but do not preserve LCSSA. 


================
Comment at: lib/Transforms/Utils/LCSSA.cpp:345
@@ +344,3 @@
+  // ported to the new PM and the legacy LoopSimplify pass has a call to
+  // mustPreserveAnalysisID(LCSSAID) in its runOnFunction method. So, for now,
+  // we are not preserving LoopSimplify.
----------------
eraman wrote:
> chandlerc wrote:
> > davide wrote:
> > > davide wrote:
> > > > mzolotukhin wrote:
> > > > > eraman wrote:
> > > > > > davide wrote:
> > > > > > > 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.
> > > > > > 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?  
> > > > > > 
> > > > > > 
> > > > > 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?
> > > > 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. 
> > > > 
> > > > 2) I think LoopSimplify should maintain LCSSA, and probably other passes in the LoopPM (which maybe don't today)
> > > To be clear: when I say LCSSA is not pure I mean the LCSSA pass in LLVM, not the canonical form per-se.
> > > 
> > > Thanks!
> > 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.
> LoopInterchange and  PPCLoopPreIncPrep.cpp call mustPreserveAnalysisID(LCSSAID) but do not preserve LCSSA. 
> 
Thanks for the summary. I have a better understanding now. Will submit with the FIXMEs removed.


http://reviews.llvm.org/D21090