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

[Davide Italiano via Phabricator via llvm-commits]

davide 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.




In https://reviews.llvm.org/D25848#624177, @davide wrote:

> In https://reviews.llvm.org/D25848#624162, @andrew.w.kaylor wrote:
>
> > What do you think of this change instead?
> >
> >   Index: lib/Transforms/Scalar/LICM.cpp
> >   ===================================================================
> >   --- lib/Transforms/Scalar/LICM.cpp	(revision 289480)
> >   +++ lib/Transforms/Scalar/LICM.cpp	(working copy)
> >   @@ -124,8 +124,13 @@
> >      }
> >   
> >      bool runOnLoop(Loop *L, LPPassManager &LPM) override {
> >   -    if (skipLoop(L))
> >   +    if (skipLoop(L)) {
> >   +      // If we have run LICM on a previous loop but now we are skipping
> >   +      // (because we've hit the opt-bisect limit), we need to clear the
> >   +      // loop alias information.
> >   +      LICM.getLoopToAliasSetMap().clear();
> >          return false;
> >   +    }
> >   
> >        auto *SE = getAnalysisIfAvailable<ScalarEvolutionWrapperPass>();
> >        return LICM.runOnLoop(L,
> >
>
>
> 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()`)


@danielcdh Dehao, ping?


https://reviews.llvm.org/D25848