[PATCH] D25434: Generate more debug info in -gmlt

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1182-1184
+  // If -fprofile-debug is enabled, need to emit the subprogram and its source
+  // location.
+  if (Asm->TM.Options.DebugInfoForProfiling || !Minimal)
----------------
dblaikie wrote:
> danielcdh wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > Another out of date comment
> > > Can this "DebugInfoForProfiling" check be floated up to the caller? (rename the 'Minimal' flag parameter to something more descriptive for the behavior change that applies to both debug-info-for-profiling and otherwise?)
> > Changed Minimal to SkipSPAttributes because there is no need to output additional attributes when DebugInfoForProfiling (thus cannot float it to caller). Does it sound OK?
> Good name - but I meant remove the testing of DebugInfoForProfiling here, but check it at the call sites?
> 
> Or perhaps that doesn't work out - multiple call sites, etc & you want to apply it in more cases. It's a while since I wrote this.
> 
> If that's the case (that it doesn't make sense to put this on one or two call sites) - could perhaps collapse it at the start of the function with something like:
> 
>   SkipSPAttributes |= Asm->TM.Options.DebugInfoForProfiling;
> 
> 
It's not about multiple-callsites, but rather the behavior is different between: "SkipSPAttributes = true" and "SkipSPAttributes = false, DebugInfoForProfiling=true" (as noted in the next comment).

I've refactored the code a little bit to make it more clear.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1195-1197
   // Skip the rest of the attributes under -gmlt to save space.
-  if (Minimal)
+  if (SkipSPAttributes)
     return;
----------------
dblaikie wrote:
> DebugInfoForProfiling shouldn't be checked here?
Right, it should not be checked here because even with -fdebug-info-for-profiling, we don't need the extra debug info which is emitted after this return.


https://reviews.llvm.org/D25434