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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added a comment.

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

> 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?


cloneBasicBlockAnalysis deleteAnalysisValue deleteAnalysisLoop are used to maintain intermediate states between nested loops. Anything that uses these function (only LICM so far) need to have special handling to clear intermediate data structure when skipLoop is called, otherwise there will be memory leak.

For the new pass manager, as from the following comment:

/// FIXME: In new pass manager, there is no helper function to handle loop
/// analysis such as cloneBasicBlockAnalysis, so the AST needs to be recomputed
/// from scratch for every loop. Hook up with the helper functions when
/// available in the new pass manager to avoid redundant computation.

The above 3 functions is not available to enable caching. So each loop is independently handled and no need to handle intermediate data structure.

Andy's change LGTM.


https://reviews.llvm.org/D25848