[PATCH] D29203: Change debug-info-for-profiling from a TargetOption to a function attribute.

Fri Jan 27 10:16:43 PST 2017

mehdi_amini accepted this revision.
mehdi_amini added inline comments.
This revision is now accepted and ready to land.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1138
+    HasDebugInfoForProfiling=true;
+
   // If there's no debug info for the function we're not going to do anything.
----------------
danielcdh wrote:
> mehdi_amini wrote:
> > danielcdh wrote:
> > > mehdi_amini wrote:
> > > > danielcdh wrote:
> > > > > mehdi_amini wrote:
> > > > > > danielcdh wrote:
> > > > > > > mehdi_amini wrote:
> > > > > > > > danielcdh wrote:
> > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > probinson wrote:
> > > > > > > > > > > danielcdh wrote:
> > > > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > > > danielcdh wrote:
> > > > > > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > > > > > ahatanak wrote:
> > > > > > > > > > > > > > > > Is debug-info-for-profiling used to turn on and off debug info for profiling on a per-function basis? I'm not very familiar with this function, but it looks like HasDebugInfoForProfiling is initialized to false in the constructor and then it will always be true after beginFunction is called on a function that has attribute debug-info-for-profiling=true.
> > > > > > > > > > > > > > > Yep that seems broken to me. If foo has the attribute and not bar, we'll emit the profiling debug info for bar if foo is processed first.
> > > > > > > > > > > > > > The issue is that I cannot check for function's attributes in DwarfUnit::applySubprogramAttributes, any suggestions on how I can pass down this attribute to that function?
> > > > > > > > > > > > > Why not do here `HasDebugInfoForProfiling = MF->getFunction()->debugInfoForProfiling();` ?
> > > > > > > > > > > > Because the llvm::DwarfDebug::beginFunction is called first before llvm::DwarfUnit::applySubprogramAttributes are called.
> > > > > > > > > > > > 
> > > > > > > > > > > > E.g. you have f1, f2, f3
> > > > > > > > > > > > 
> > > > > > > > > > > > the call sequence is:
> > > > > > > > > > > > beginFunction(f1)
> > > > > > > > > > > > beginFunction(f2)
> > > > > > > > > > > > beginFunction(f3)
> > > > > > > > > > > > applySubprogramAttributes(subprogramof_f1)
> > > > > > > > > > > > applySubprogramAttributes(subprogramof_f2)
> > > > > > > > > > > > applySubprogramAttributes(subprogramof_f3)
> > > > > > > > > > > > 
> > > > > > > > > > > > The current logic is, if there is any function that has HasDebugInfoForProfiling=true, then all function will have it.
> > > > > > > > > > > All the more reason to put it at module or CU level, rather than function level.
> > > > > > > > > > If the behavior you want is to have it enabled for the module, then yes like @probinson  says a module level construct would make more sense.
> > > > > > > > > > 
> > > > > > > > > > I don't enough about `-gmlt` to know what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -gmlt ?
> > > > > > > > > > 
> > > > > > > > > Updated to use module flag for this.
> > > > > > > > Can you clarify what is the behavior in this case:
> > > > > > > > 
> > > > > > > > > what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -gmlt ?
> > > > > > > I am not debug info expert, Eric may have more accurate answer.
> > > > > > > 
> > > > > > > My understanding is that one will have more debug info emitted, the other will only have line table emitted. What will happen in LTO?
> > > > > > I'm asking because I haven ming that of `DebugInfoForProfiling ` will be enabled by `-gmlt`, but that may not be true?  If so when/how is  `DebugInfoForProfiling ` set?
> > > > > It will be set with -fdebug-info-for-profiling.
> > > > > 
> > > > > A follow-up cfe patch is in: https://reviews.llvm.org/D29205
> > > > Ok so back to my question:
> > > > 
> > > > > what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -fdebug-info-for-profiling? How does your module flag affect that?
> > > > 
> > > -fdebug-info-for-profiling is orthogonal with -g/-gmlt. So I guess the question should be:
> > > 
> > > what we would expect if we have a.cpp and b.cpp and we build with LTO, a.cpp with -fdebug-info-for-profiling and b.cpp without the flag? How does your module flag affect that?
> > > 
> > > The answer is
> > > * a.cpp will have the flag and thus all the logic guarded by this flag in backend will be executed.
> > > * b.cpp will not have it and will not have related code executed.
> > > * If some functions are imported from b.cpp to a.cpp, it will inherit the flag from a.cpp thus have related code executed.
> > > * If some functions are imported from a.cpp to c.cpp, it will inherit the flag from b.cpp thus not have related code executed.
> > > 
> > > I guess this also applies to the cases that a.cpp is built with -g and b.cpp is built with -gmlt.
> > During Monolithic LTO, the two files are *merged*. There is a single CodeGen happening.
> > 
> > I'm still trying to figure out what is the impact of `If some functions are imported from b.cpp to a.cpp, it will inherit the flag from a.cpp thus have related code executed`, because I don't know enough about `-fdebug-info-for-profiling`.
> > 
> > I think I originally misread this comment:
> > 
> > ```
> >   // Under -gmlt, skip building the subprogram if there are no inlined
> >   // subroutines inside it. But with -fdebug-info-for-profiling, the subprogram
> >   // is still needed as we need its source location.
> > ```
> > 
> > If I understand correctly, the module flag would make the debug info emission more conservative. Does it mean that adding `-fdebug-info-for-profiling` with `-g` does not have any effect on the Dwarf emission? It is only in effect with `DICompileUnit::LineTablesOnly`?
> > 
> > 
> -fdebug-info-for-profiling has the following implication:
> 
> 1. emit extra subprogram info (e.g. decl_line) even with -gmlt
> 2. emit discriminator and encode optimization effects to discriminator.
> 
> For #1, -g also emits these extra info, thus it only applies to -gmlt
> For #2, which is under review in https://reviews.llvm.org/D26420, it applies to both -g and -gmlt builds.
> 
Thanks for the explanation!
With this I'm fine with the module flag. 

If @probinson does not have any other comment that LGTM.

https://reviews.llvm.org/D29203