[PATCH] D19397: Initial patch for inlining report

[Hal Finkel via llvm-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D19397#447644, @rcox2 wrote:

> In http://reviews.llvm.org/D19397#440168, @hfinkel wrote:
>
> > In http://reviews.llvm.org/D19397#436898, @rcox2 wrote:
> >
> > > Hal,
> > >
> > > I’ve been looking at the patch that you posted for the annotated source listing to get an idea of how you would like the inlining report to conform to the diagnostic mechanism which you have already implemented.
> > >
> > > It seems to me that you would like the information needed to construct the inlining report to be passed back to the diagnostic mechanism via function calls like:
> > >
> > > emitOptimizationRemark()
> > >
> > > when the inlining occurs, and
> > >
> > > emitOptimizationRemarkMissed()
> > >
> > > when the inlining does not occur.
> >
> >
> > Yes (although you should use a proper subclass to store all of the relevant information in member variables). The other thing to track is the conversation about serialization of these things into YAML for consumption by external tools.
>
>
> OK, so, for example, in DiagnosticInfo.h I see:
>
> class DiagnosticInfoOptimizationRemark : public DiagnosticInfoOptimizationBase {
>
>   ...
>
> }
>
> and in DiagnosticInfo.cpp, I see:
>
> void llvm::emitOptimizationRemark(LLVMContext &Ctx, const char *PassName,
>
>                                   const Function &Fn, const DebugLoc &DLoc,
>                                   const Twine &Msg) {
>   Ctx.diagnose(DiagnosticInfoOptimizationRemark(PassName, Fn, DLoc, Msg));
>
> }
>
> and in Inliner.cpp., I see a call:
>
>   emitOptimizationRemark(
>        CallerCtx, DEBUG_TYPE, *Caller, DLoc,
>        Twine(Callee->getName() + " inlined into " + Caller->getName()));
>   
>   
>
> To extend this, I would declare a subclass:
>
> class DiagnosticInfoInlineOptimizationRemark: public  DiagnosticInfoOptimizationRemark  {
>
>   ...
>
> }
>
> Add a definition:
>
> void llvm::emitOptimizationInlineRemark(LLVMContext &Ctx,
>
>                                   const CallSite& CS, const DebugLoc &DLoc,
>                                   const Twine &Msg) {
>   Ctx.diagnose(DiagnosticInfoOptimizationRemark(CS, DLoc, Msg));
>
> }
>
> and then replace the call to emitOptimizationRemark() in Inliner.cpp with a call to emitOptimizationInlineRemark();


Yes, that sounds about right. I don't think we need emitOptimizationInlineRemark(), the inliner should probably just call diagnose directly with its newly-created DiagnosticInfoInlineOptimizationRemark object. The DiagnosticInfoInlineOptimizationRemark will likely need a bunch of constructor parameters anyway to capture all of the various inlining details.

> Let me know if I have this right.

> 

> > 

> 

> > 

> 

> > > Then, the report could be emitted in a manner similar to how you called OptReportAction::GenerateReportFile() for the annotated source listing.

> 

> > 

> 

> > 

> 

> > Yes, although it sounds like the chosen path is to dump all of the information into YAML files, and then have the actual report generation in external tools that can consume those files, and the sources, to produce reports.

> 

> 

> So, I'm trying to understand if we would go through the process of generating a YAML even when we need YAML for an external tool or not.

> 

> What are your thoughts?


I don't understand your question. We'll want some general infrastructure for serializing all of the remarks into YAML, although we don't have it yet.

Thanks again, Hal

> 

> 

> > > Am I on the right track?

> 

> > 

> 

> > > 

> 

> > 

> 

> > > To do so, I would need to pass down more information than is currently being passed down.  I would need at least something that identifies the callsite either being inlined or not inlined, and, when reasons are added, the reason (or reasons) the inlining did or didn’t happen.

> 

> > 

> 

> > 

> 

> > You mean into those functions? Yes, you'd need to use different functions (or directly create the remark classes and then directly call 'diagnose' with that class).

> 

> > 

> 

> > > Would I just add another parameter pointer to a class variable to do that?

> 

> > 

> 

> > > 

> 

> > 

> 

> > > - Thanks, Robert Cox

> 

> > 

> 



http://reviews.llvm.org/D19397