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

[serge via Phabricator via llvm-commits]

serge-sans-paille marked 2 inline comments as done.
serge-sans-paille added inline comments.


================
Comment at: llvm/include/llvm/Pass.h:216
+  AnalysisType &getAnalysisID(AnalysisID PI, Function &F,
+                              bool *Changed = nullptr);
 };
----------------
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


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

https://reviews.llvm.org/D81236