[PATCH] D21771: [OptRemark] RFC: Add hotness attribute

[Hal Finkel via llvm-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D21771#474636, @anemet wrote:

> In http://reviews.llvm.org/D21771#474604, @hfinkel wrote:
>
> > Is there a reason that these should be free functions instead of just making an Analysis pass with these as member functions? Making it an actual analysis will make it easy to require BFI. I think the "get if available" will end up being confusing (sometimes the filtering will work, sometimes not, depending on what gets invalidated when - especially with the new pass manager where this will be dynamic). Side note: We might want to make BFI lazy (or compute-on-first-query instead of actually computing things in runOnFunction).
>
>
> Thanks for feedback!
>
> The main reason these are free function so that it's easy to transition from the current API which uses free function (i.e. no major reason ;-).  But yes, I wasn't completely happy with the BFI dependence either.  It was working for LV that already depends on BFI but it didn't work for LoopDist for example.  I added a new command flag and made the BFI dependence conditional on that.
>
> How would an analysis pass help this.  It feels orthogonal because we can't unconditionally require the new analysis either because it would populate BFI.  I actually think that the new PM will help here because we could then hopefully check F.getEntryCount to see if PGO is available and only query BFI then, no?
>
> Thinking a bit more, having this packaged as a new analysis pass may help because right now I have this in LoopDist for example:
>
> +    if (PassRemarksWithHotness)      <------ this is the command line option
>  +      AU.addRequired<BlockFrequencyInfoWrapperPass>();
>
> but when the emitOptimizationRemark* function is called, I don't know if BFI is actually available (i.e. whether the pass was updated with the change above) so I need to use getAnalysisIfAvailable.  Having the Analysis pass would take care of this because it's all under the control of the analysis pass.


Exactly. Looks like we both agree on packaging this as an analysis pass...

> Regarding the BFI population, I was thinking of using the command line flag (-pass-remarks-with-hotness).  Hopefully we can somehow detect if PGO is available and then enable the flag automatically at some higher level.  As you say, lazy BFI would be helpful but it would be nice not to directly depend on that feature for this project.

> 

> What do you think?


I think we should add some global state (perhaps something that is set when setDiagnosticHandler is called, or via some nearby API) that indicates whether diagnostic "hotness" is requested. We should package these wrappers as an analysis pass, and that pass should have a hard requirement on BFI.

Regarding making BFI lazy, I think we probably need to do this, at least in the trivial sense: Add a Boolean to BFI indicating whether or not 'calculate' has been called, and add 'if (!Calculated) calculate()' to BFI::getBlockFreq, BFI::getBlockProfileCount and BFI::setBlockFreq, remove the call to calculate in runOnFunction. There is a more-complicated sense of making BFI lazy -- only computing frequencies for parts of the function as required -- but that's a larger project.

Then if we make no BFI calls, then requiring it is free. I think then the infrastructure will just work for us.

> Adam



http://reviews.llvm.org/D21771