[PATCH] D25848: [PM/OptBisect] Don't crash with some particular values of -opt-bisect-limit=

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D25848#624324, @andrew.w.kaylor wrote:

> In https://reviews.llvm.org/D25848#624177, @davide wrote:
>
> > I consider this reasonable, but I don't feel qualified enough to review as I'm not a LICM expert, so I'm CC:ing @danielcdh  . I'm slightly worried about other passes hitting a similar issue (expecting some state to be freed and asserting in `doFinalization()`)
>
>
> I think that's a reasonable concern, but I'm inclined to believe that this occurring is an indication of sloppy design in such passes that just happens to be exposed by OptBisect.  For instance, the reason the problem surfaced in this case is that one instance of the LICM pass was creating state that it expected to be cleaned up by a later instance of the same pass.  This kind of cross-pass dependency really should be avoided.  I'm not entirely clear how LICM is using this information, but it makes me wonder if LICM should be a function pass, since it is apparently operating beyond the scope of a single loop.


I agree with Andy here.

Fundamentally, LICM is playing *very* fast and loose with the pass infrastructure. bisect is catching it, good! We can't really fix it in the current pass manager. =[ So the best we can do is hack around it, and I think Andy's suggested hack is reasonable. I think this will be fairly rare at least, and the new pass manager gives us options to specifically avoid this design trap.

The key issue is that LICM wants to save state from one loop to use in the next. If we had, say, a LoopAnalysisManager (as we have in the new PM) which could cache such state and provide it later on, this would Just Work with no weird state buried in the pass.

Anyways, Andy's proposed patch LGTM. Davide, if you want to clean it up and land it, go for it. No need for more review here, just explain what is going on. Thanks for chasing this down1


https://reviews.llvm.org/D25848