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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


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


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

https://reviews.llvm.org/D81236