Page MenuHomePhabricator

[CodeView] Add CodeView support for PGO debug information
ClosedPublic

Authored by Holman on Apr 6 2021, 3:16 PM.

Details

Summary

This change adds debug information about whether PGO is being used or not.

Microsoft performance tooling (e.g. xperf, WPA) uses this information to show whether functions are optimized with PGO or not, as well as whether PGO information is invalid.

This information is useful for validating whether training scenarios are providing good coverage of real world scenarios, showing if profile data is out of date, etc.

Diff Detail

Event Timeline

Holman created this revision.Apr 6 2021, 3:16 PM
Holman requested review of this revision.Apr 6 2021, 3:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 6 2021, 3:16 PM

think this looks good overall; maybe it should also have an IR to codeview test?

rnk added a subscriber: rnk.Apr 8 2021, 12:14 PM

IMO it's best to avoid adding fields to DICompileUnit if at all possible. It's the "god object" / "katamari damacy" of module debug info. Is there something about the IR module that indicates if PGO data is present or not? We could check that instead. I looked, but I wasn't able to find anything quickly.

Holman added a comment.Apr 8 2021, 1:11 PM
In D99994#2677566, @rnk wrote:

IMO it's best to avoid adding fields to DICompileUnit if at all possible. It's the "god object" / "katamari damacy" of module debug info. Is there something about the IR module that indicates if PGO data is present or not? We could check that instead. I looked, but I wasn't able to find anything quickly.

Got it. It looks like MMI->getModule()->getProfileSummary() should have the info I need. I'll update to use that instead.

Holman updated this revision to Diff 336223.Apr 8 2021, 2:07 PM

Get PGO info from Module instead of adding new field to debug info.

rnk added a comment.Apr 8 2021, 2:12 PM

Seems reasonable, but we still need a small IR test.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
807

nit, the more common style to name parameters looks like getProfileSummary(/*IsCS=*/false). Sort of more Python-y.

1434

I think the preferred spelling is GV.hasProfileData(), I think that's functionally equivalent.

Holman updated this revision to Diff 336274.Apr 8 2021, 4:53 PM
Holman set the repository for this revision to rG LLVM Github Monorepo.

Add a test

Holman marked 2 inline comments as done.Apr 8 2021, 4:54 PM
Holman updated this revision to Diff 336497.Apr 9 2021, 9:42 AM

Fix clang-format issue.

rnk accepted this revision.Apr 9 2021, 2:27 PM

lgtm, thanks!

This revision is now accepted and ready to land.Apr 9 2021, 2:27 PM

Can someone help me get this checked in?

This revision was landed with ongoing or failed builds.Wed, Apr 21, 3:34 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Wed, Apr 21, 3:34 PM

Can someone help me get this checked in?

Sure, I went ahead and pushed it.