This is an archive of the discontinued LLVM Phabricator instance.

Add -fdebug-info-for-profiling to emit more debug info for sample pgo profile collection
ClosedPublic

Authored by danielcdh on Oct 10 2016, 8:35 AM.

Details

Summary

SamplePGO uses profile with debug info to collect profile. Unlike the traditional debugging purpose, sample pgo needs more accurate debug info to represent the profile. We add -femit-accurate-debug-info for this purpose. It can be combined with all debugging modes (-g, -gmlt, etc). It makes sure that the following pieces of info is always emitted:

  • start line of all subprograms
  • linkage name of all subprograms
  • standalone subprograms (functions that has neither inlined nor been inlined)

The impact on speccpu2006 binary size (size increase comparing with -g0 binary, also includes data for -g binary, which does not change with this patch):

-gmlt(orig) -gmlt(patched) -g

433.milc 4.68% 5.40% 19.73%
444.namd 8.45% 8.93% 45.99%
447.dealII 97.43% 115.21% 374.89%
450.soplex 27.75% 31.88% 126.04%
453.povray 21.81% 26.16% 92.03%
470.lbm 0.60% 0.67% 1.96%
482.sphinx3 5.77% 6.47% 26.17%
400.perlbench 17.81% 19.43% 73.08%
401.bzip2 3.73% 3.92% 12.18%
403.gcc 31.75% 34.48% 122.75%
429.mcf 0.78% 0.88% 3.89%
445.gobmk 6.08% 7.92% 42.27%
456.hmmer 10.36% 11.25% 35.23%
458.sjeng 5.08% 5.42% 14.36%
462.libquantum 1.71% 1.96% 6.36%
464.h264ref 15.61% 16.56% 43.92%
471.omnetpp 11.93% 15.84% 60.09%
473.astar 3.11% 3.69% 14.18%
483.xalancbmk 56.29% 81.63% 353.22%
geomean 15.60% 18.30% 57.81%

Debug info size change for -gmlt binary with this patch:

433.milc 13.46%
444.namd 5.35%
447.dealII 18.21%
450.soplex 14.68%
453.povray 19.65%
470.lbm 6.03%
482.sphinx3 11.21%
400.perlbench 8.91%
401.bzip2 4.41%
403.gcc 8.56%
429.mcf 8.24%
445.gobmk 29.47%
456.hmmer 8.19%
458.sjeng 6.05%
462.libquantum 11.23%
464.h264ref 5.93%
471.omnetpp 31.89%
473.astar 16.20%
483.xalancbmk 44.62%
geomean 16.83%

Event Timeline

danielcdh updated this revision to Diff 74133.Oct 10 2016, 8:35 AM
danielcdh retitled this revision from to Generate more debug info in -gmlt.
danielcdh updated this object.
danielcdh added reviewers: dblaikie, echristo, davidxl.
danielcdh added a subscriber: llvm-commits.
probinson added inline comments.
lib/CodeGen/CGDebugInfo.cpp
2748

Need to update the comment to match how the code changed.

danielcdh updated this revision to Diff 79609.Nov 29 2016, 11:21 AM

Add -femit-accurate-debug-info flag to guard the change.

danielcdh retitled this revision from Generate more debug info in -gmlt to Add -femit-accurate-debug-info to emit more debug info for sample pgo profile collection.Nov 29 2016, 11:26 AM
danielcdh updated this object.
danielcdh added a subscriber: cfe-commits.
danielcdh updated this revision to Diff 79629.Nov 29 2016, 1:33 PM
danielcdh marked an inline comment as done.

Change the flag to -fprof-debug, which is more concise. The flag name is still open for discussion.

Change the flag to -fprof-debug, which is more concise. The flag name is still open for discussion.

Well, since I have permission to bikeshed... :)

I'd prefer to have 'profile' rather than 'prof' in the name. This fits the established convention with -fprofile-sample-use etc.

danielcdh updated this revision to Diff 79770.Nov 30 2016, 9:49 AM

change the flag name to -fprofile-debug

echristo added inline comments.Dec 5 2016, 6:03 PM
lib/CodeGen/CGDebugInfo.cpp
2745–2748

Should we be encapsulating all of these for profile debug info? I.e. I think coverage analysis is going to want the same things.

danielcdh added inline comments.Dec 5 2016, 6:56 PM
lib/CodeGen/CGDebugInfo.cpp
2745–2748

Do you mean that -fcoverage also implies -fprofile-debug?

I think the reason we introduce -fprofile-debug is that it has different requirments for debug info than coverage/sanitizer. E.g. we want to emit discriminator for -fprofile-debug, but not coverage/sanitizer.

echristo added inline comments.Dec 9 2016, 6:40 PM
lib/CodeGen/CGDebugInfo.cpp
2745–2748

So, what are the differences here? I imagine that profile debugging will want accurate source information. Perhaps we should hash this out in the thread that dblaikie has started on llvm-dev.

danielcdh added inline comments.Dec 9 2016, 7:27 PM
lib/CodeGen/CGDebugInfo.cpp
2745–2748

The discussion about introducing this flag is in the thread of http://lists.llvm.org/pipermail/llvm-dev/2016-November/107645.html

Basically -fprofile-debug not only requires accurate source information, it also requires:

  1. emit linkage name in all subprograms
  2. emit discriminator with dup-factor copy-factor encoded
  3. use-unknown-locations=true

The above 3 items are not required by -fcoverage. That's why we introduce -fprofile-debug to record these extra debug info.

change the flag name to -fprofile-debug

I don't really like this name because it sounds like it might be enabling some kind of debugging of the profiling. How about -fdebug-info-for-profiling. Another options is to make it a mode for -g such as -gprofiling.

change the flag name to -fprofile-debug

I don't really like this name because it sounds like it might be enabling some kind of debugging of the profiling. How about -fdebug-info-for-profiling. Another options is to make it a mode for -g such as -gprofiling.

Thanks for the comment. I'm OK with -fdebug-info-for-profiling. If no one objects, I'll update the patch tomorrow morning.

About -gprofiling, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2016-November/107454.html, this should be a -f option instead of -g option.

danielcdh updated this revision to Diff 81609.Dec 15 2016, 10:13 AM

update option name

ping...

Thanks,
Dehao

dblaikie accepted this revision.Jan 9 2017, 11:10 AM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Jan 9 2017, 11:10 AM
danielcdh retitled this revision from Add -femit-accurate-debug-info to emit more debug info for sample pgo profile collection to Add -fdebug-info-for-profiling to emit more debug info for sample pgo profile collection.Jan 9 2017, 11:23 AM
danielcdh edited edge metadata.

Thanks David for the reivew.

Could you also help take a look at https://reviews.llvm.org/D25434, as it depends on the TargetOptions.h change in that patch.

Thanks,
Dehao

echristo edited edge metadata.Jan 9 2017, 11:51 AM

I don't think this ever was hashed out in the llvm-dev thread?

echristo accepted this revision.Jan 18 2017, 4:28 PM

Thanks for explaining all of this and going through it Dehao.

LGTM.

-eric

danielcdh closed this revision.Jan 18 2017, 4:55 PM