Page MenuHomePhabricator

[memprof] Print out the summary in YAML format.
ClosedPublic

Authored by snehasish on Jan 6 2022, 5:43 PM.

Details

Summary

Print out the profile summary in YAML format to make it easier to for
tools and tests to read in the contents of the raw profile.

Diff Detail

Event Timeline

snehasish created this revision.Jan 6 2022, 5:43 PM
snehasish requested review of this revision.Jan 6 2022, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 5:43 PM
tejohnson added inline comments.Jan 7 2022, 8:38 AM
llvm/lib/ProfileData/RawMemProfReader.cpp
106–107

Add a comment here and/or at the declaration that this is printing in YAML format.

llvm/tools/llvm-profdata/llvm-profdata.cpp
2491

Can this be moved into printSummaries?

snehasish updated this revision to Diff 398237.Jan 7 2022, 2:19 PM

Address comments.

PTAL, thanks!

I would really like to have a test to ensure that the YAML we generate is valid but it seems hard to implement without defining a document type to parse into as a unittest (see https://llvm.org/docs/YamlIO.html#input). Alternatively, I could also extend the lit based test to check if YAML is available like this usage in MLIR (https://git.io/J9ORA). Any suggestions?

llvm/lib/ProfileData/RawMemProfReader.cpp
106–107

Documented the format in the header.

llvm/tools/llvm-profdata/llvm-profdata.cpp
2491

Added a new printYAML method and moved the `OS << "memprof_profile:" statement there.

PTAL, thanks!

I would really like to have a test to ensure that the YAML we generate is valid but it seems hard to implement without defining a document type to parse into as a unittest (see https://llvm.org/docs/YamlIO.html#input). Alternatively, I could also extend the lit based test to check if YAML is available like this usage in MLIR (https://git.io/J9ORA). Any suggestions?

I don't have a strong feeling. Looks like the tests that dump opt remarks to yaml format just manually check that the output lines look as expected but don't attempt to parse them via yaml itself.

llvm/tools/llvm-profdata/llvm-profdata.cpp
2491

Is there a reason for not wanting to put it at the top of printSummaries?

snehasish updated this revision to Diff 399786.Jan 13 2022, 2:18 PM

Convert the snake case yaml to camel case with initcap (matching llvm variable naming convention). This makes it easy to use macro based expansion to generate the meminfoblock contents in subsequent patches.

PTAL, thanks!

I would really like to have a test to ensure that the YAML we generate is valid but it seems hard to implement without defining a document type to parse into as a unittest (see https://llvm.org/docs/YamlIO.html#input). Alternatively, I could also extend the lit based test to check if YAML is available like this usage in MLIR (https://git.io/J9ORA). Any suggestions?

I don't have a strong feeling. Looks like the tests that dump opt remarks to yaml format just manually check that the output lines look as expected but don't attempt to parse them via yaml itself.

Ok, let me punt on this for now.

I've changed the YAML naming from snake case to match the LLVM variable naming style to keep it consistent with the output from the macro based implementation in D117256.

PTAL, thanks!

llvm/tools/llvm-profdata/llvm-profdata.cpp
2491

Yes, the MemprofProfile marks the outermost object which holds the rest of the information incl. the header information (from printSummaries) and the contents of the raw profile (functionality added in D116784). I decided to introduce the printYAML method in this patch rather than have the print inside printSummaries and then pull it out into a separate method in the next one where we print out the contents of the profile. LMK if you feel strongly about keeping it in the printSummaries method.

tejohnson accepted this revision.Jan 15 2022, 12:59 PM

lgtm

llvm/tools/llvm-profdata/llvm-profdata.cpp
2491

Ah, I missed the fact that D116784 adds more to the outer printYAML. This looks fine then.

This revision is now accepted and ready to land.Jan 15 2022, 12:59 PM
This revision was automatically updated to reflect the committed changes.