[PATCH] D81236: Improve LegacyPassManager API to correctly report modified status
Ehud Katz via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 25 10:14:13 PDT 2020
ekatz accepted this revision.
ekatz added a comment.
This revision is now accepted and ready to land.
I admit, I don't like this change. This is a work around a design flaw in the old PM. However, it seems like the right decision to handle it, right now.
I think we should post a RFC for a new - *cough* - PM... ;)
LGTM, but please separate the patch into two: the PM related; and the LoopExtractor.
================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:135
+ bool Changed = false;
+ LoopInfo &LI = getAnalysis<LoopInfoWrapperPass>(F, &Changed).getLoopInfo();
----------------
serge-sans-paille wrote:
> ekatz wrote:
> > jdoerfert wrote:
> > > ekatz wrote:
> > > > Maybe I am missing something, but isn't `LoopExtractor` dependent on `BreakCriticalEdges`? I mean, what are the transformations that the `LoopInfo` analysis depends on?
> > > > what are the transformations that the LoopInfo analysis depends on?
> > >
> > > No transformations:
> > >
> > > ```
> > > void LoopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
> > > AU.setPreservesAll();
> > > AU.addRequiredTransitive<DominatorTreeWrapperPass>();
> > > }
> > >
> > > ```
> > So why would the `Changed` ever be set to `true`?
> It's set to True because of the on the fly pass evaluation: asking for `LoopInfo` triggers the whole dependencies, including `BreakCriticalEdges`. That's why the API change I propose only impacts on the fly passes.
I see.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81236/new/
https://reviews.llvm.org/D81236
More information about the llvm-commits
mailing list