[PATCH] D81236: Improve LegacyPassManager API to correctly report modified status

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Pass.h:216
+  AnalysisType &getAnalysisID(AnalysisID PI, Function &F,
+                              bool *Changed = nullptr);
 };
----------------
mehdi_amini wrote:
> serge-sans-paille wrote:
> > jdoerfert wrote:
> > > mehdi_amini wrote:
> > > > serge-sans-paille wrote:
> > > > > mehdi_amini wrote:
> > > > > > serge-sans-paille wrote:
> > > > > > > mehdi_amini wrote:
> > > > > > > > Can you document these added arguments? 
> > > > > > > > It isn't clear to me what are these about: I wouldn't expect an analysis to change the IR for example.
> > > > > > > Will do. That's all the salt of this review: an analysis may trigger, as part of its dependencies, a pass that modifies the IR. This modification is currently ignored when computing on-the-fly passes.
> > > > > > Do you have examples of it? (if so maybe add them to the comment)
> > > > > Yeah, LoopExtractor depends on BreakCriticalEdges. I'll add that to the comment.
> > > > Is this intended to work though? What about the new pass manager? Do we support analysis depending on IR transformations?
> > > LoopExtractor is not an analysis but a transformation. In `lib/Analysis` there is no occurrence of `addRequiredID`, in `lib/Transforms` there are 11 files. This is the common way to depend on IR transformations (I'd argue it is intended to work). The need in the new PM is reduced as most of these things depend on loop-simplify and lcssa, both provided implicitly by the new PM loop wrapper pass (I think). The other uses cases seem to not have a new PM implementation yet.
> > +1 @jdoerfert It's even explicitly mentioned in https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Transforms/Utils.h#L68
> > . This is the common way to depend on IR transformations (I'd argue it is intended to work)
> 
> Right, so my memory of the old pass manager is that you indeed declare dependencies ahead of time, like the comment Serge is pointing at:
> 
> ```
>  AU.addRequiredID(BreakCriticalEdgesID);
> ```
> 
> However what we have here is that after a transformation started (`LoopExtractor::runOnModule(Module &M)`) we need an *analysis* and we call `getAnalysis`.
> 
> It seems strange to me that a call to `getAnalysis` is *transforming* the IR in the middle of the pass here. 
> It seems strange to me that a call to getAnalysis is *transforming* the IR in the middle of the pass here.

I'm not saying it is particularly sane. TBH, I *really hope* this is a temporary fix for an end-of-life part of the code base. Requiring major changes to the old-PM seems both unreasonable and dangerous too me. Just ignoring this is also not a great option. This patch gives us a reasonable way to deal with this "feature" of the old-PM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81236/new/

https://reviews.llvm.org/D81236