This is an archive of the discontinued LLVM Phabricator instance.

[InstrProf] Attach debug info to counters
ClosedPublic

Authored by ellis on Nov 24 2021, 2:09 PM.

Details

Summary

Add the llvm flag -debug-info-correlate to attach debug info to instrumentation counters so we can correlate raw profile data to their functions. Raw profiles are dumped as .proflite files. The next diff enables llvm-profdata to consume .proflite and debug info files to produce a normal .profdata profile.

Part of the "lightweight instrumentation" work: https://groups.google.com/g/llvm-dev/c/r03Z6JoN7d4

Diff Detail

Event Timeline

ellis created this revision.Nov 24 2021, 2:09 PM
ellis edited the summary of this revision. (Show Details)Nov 24 2021, 2:20 PM
ellis edited the summary of this revision. (Show Details)
ellis published this revision for review.Nov 29 2021, 5:05 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptNov 29 2021, 5:05 PM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits. · View Herald Transcript
kyulee added inline comments.Nov 29 2021, 5:54 PM
compiler-rt/lib/profile/InstrProfilingWriter.c
274

Is it no diff change? I wonder if there is a case where CountersSize is 0 but not DataSize from a value probe.
If not, should we make a separate change?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
649

It appears this is a NFC. Should we make a separate change?

kyulee added inline comments.Nov 30 2021, 9:58 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
851

It checks an expression but appears to apply the constant case below. Should it check a constant expression, instead?

ellis added inline comments.Nov 30 2021, 3:27 PM
compiler-rt/lib/profile/InstrProfilingWriter.c
274

I wonder if there is a case where CountersSize is 0 but not DataSize from a value probe.

Ah, yes I believe this is possible. I'll fix this here.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
851

Oh we actually don't need to use DIExpression at all. Instead we can emit ConstantAsMetadata to simplify it quite a bit.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
649

Yeah I can move this out of this patch.

ellis updated this revision to Diff 390855.Nov 30 2021, 4:41 PM

Emit metadata as ConstantAsMetadata instead of the more complicated DIExpression.

kyulee added inline comments.Dec 2 2021, 5:26 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
968

I think when -debug-info-correlate is used without debug info, we should fail or emit a warning here instead of silently proceeding it because we cannot correlate it anyhow down the road.

ellis added inline comments.Dec 3 2021, 2:58 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
968

I'll emit a warning. I don't want to fail in this case when a single function doesn't have debug info. Instead, I'm planning on adding a check in the frontend when parsing the -fprofile-generate-correlate=debug-info flag (which I will add later).

ellis updated this revision to Diff 391780.Dec 3 2021, 4:49 PM

Add warning when debug info is missing.

kyulee added inline comments.Dec 5 2021, 11:47 AM
compiler-rt/lib/profile/InstrProfilingWriter.c
306

Should it be down after __llvm_write_binary_ids below although it doesn't do anything in general?
I saw D114566 seems to parse this to compute the other addresses.

ellis marked 5 inline comments as done.Dec 7 2021, 10:50 AM
ellis updated this revision to Diff 392471.Dec 7 2021, 10:51 AM

Add a proper warning and simplify compiler-rt profile writer.

This change for the profile writer looks good to me!
Before signing it off, I'd like to hear any other suggestions or opinions from @davidxl @MaskRay @alanphipps since this is technically the very first change.

ellis updated this revision to Diff 392576.Dec 7 2021, 3:47 PM

Fix MachO profile dumping code.

kyulee accepted this revision.Dec 13 2021, 2:31 PM
This revision is now accepted and ready to land.Dec 13 2021, 2:31 PM
ellis updated this revision to Diff 394097.Dec 13 2021, 5:43 PM

Rebase and test.

This revision was landed with ongoing or failed builds.Dec 13 2021, 5:51 PM
This revision was automatically updated to reflect the committed changes.