[PATCH] D82270: Added a Printer to the FunctionPropertiesAnalysis

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D82270#2112560 <https://reviews.llvm.org/D82270#2112560>, @jdoerfert wrote:

> In D82270#2110007 <https://reviews.llvm.org/D82270#2110007>, @mtrofin wrote:
>
> > In D82270#2109923 <https://reviews.llvm.org/D82270#2109923>, @jdoerfert wrote:
> >
> > > In D82270#2106860 <https://reviews.llvm.org/D82270#2106860>, @mtrofin wrote:
> > >
> > > > What's the value of having the result type have the analyze method? Is there a scenario where we want to get the result and then re-analyze it?
> > >
> > >
> > > In the new-PM if you preserve the analysis it doesn't need to be recomputed, I thought. Or maybe I misunderstand your question. Could you elaborate?
> > >
> > > [EDIT: Do you mean the `...Info` struct? It is used by two passes, right?]
> >
> >
> > What I mean was, why not let the computation happen in the Analysis::run, and the Info is basically an object not meant to be mutated. This is because the PM will call ::run to recompute, and we don't really have (or want to have) a scenario where we want someone to call Info::analyze() (unless I'm missing something)
>
>
> Oh, I see. This is a common pattern, I think. Mostly because it allows the passes to be minimal wrappers. This is good as we might want to have an old-PM pass as well. Then you want the analyze in the info "for sure". Even without an old-PM pass, this way allows old-PM cgscc passes to create the info object and populate it, something that doesn't work in the old-PM through the pass mechanism.
>
> Is there a strong reason or is it more a design preference?
>
> In D82270#2112462 <https://reviews.llvm.org/D82270#2112462>, @mtrofin wrote:
>
> > In D82270#2112379 <https://reviews.llvm.org/D82270#2112379>, @jdoerfert wrote:
> >
> > > In D82270#2111884 <https://reviews.llvm.org/D82270#2111884>, @mtrofin wrote:
> > >
> > > > Sounds like we need the Info's self-refreshing ability to support old-PM scenarios, correct? If I understand it correctly, there, the Info object could be retrieved from the analysis (this'd be an old-PM analysis, to be added), and then the client could refresh it without the involvement of the analysis or analysis manager, but updating the state the Analysis manager. Is that even desirable?
> > >
> > >
> > > Its for the old-PM scenario but there is no need for the result to be self-refreshing. The key point is that the analyze method is shared, thus not part of the new-PM pass. If we want a immutable result we can again create a `Result` class in the `FunctionPropertiesInfo`. I don't mind that either way as long as `analyze` (and `print`) logic is outside of `FunctionPropertiesAnalysis` I'm happy. Does that make sense?
> >
> >
> > could it be shared by having it as a static (basically, factory method) on FunctionPropertiesInfo?
>
>
> sure.


deal :)

I think it'd still make sense to split the patch one chunk to refactor (NFC), one to add the printer with test and all that - wdyt?


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

https://reviews.llvm.org/D82270