This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO][llvm-profgen] Renovate perfscript check and command line input validation
ClosedPublic

Authored by wlei on Feb 9 2021, 4:57 PM.

Details

Summary

This include some changes related with PerfReader's the input check and command line change:

  1. It appears there might be thousands of leading MMAP-Event line in the perfscript for large workload. For this case, the 4k threshold is not eligible to determine it's a hybrid sample. This change renovated the isHybridPerfScript by going through the script without threshold limitation checking whether there is a non-empty call stack immediately followed by a LBR sample. It will stop once it find a valid one.
  1. Added several input validations for the command line switches in PerfReader.
  1. Changed the command line show-disassembly to show-disassembly-only, it will print to stdout and exit early which leave an empty output profile.

Diff Detail

Event Timeline

wlei created this revision.Feb 9 2021, 4:57 PM
wlei requested review of this revision.Feb 9 2021, 4:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 4:57 PM
wlei edited the summary of this revision. (Show Details)Feb 9 2021, 5:15 PM
wlei added reviewers: wmi, hoy, wenlei, davidxl.
wenlei added inline comments.Feb 10 2021, 10:27 AM
llvm/tools/llvm-profgen/PerfReader.h
526

A comment showing expected input would be helpful. For LBR part, do we want to check more than leading 0x?

Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?

wlei added inline comments.Feb 10 2021, 11:48 AM
llvm/tools/llvm-profgen/PerfReader.h
526

Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?

Do you mean we should have three state for that?

  1. LBR + call stack, which is detected in this function.
  2. LBR only (when we detect the LBR but Count is zero)
  3. no LBRs at all.

For the 3) case, we will emit an error.

hoy added inline comments.Feb 10 2021, 12:00 PM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
404

nit: isShowDisassemblyEnabled

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

We should probably return here with --show-disassembly and the output file should have the disassemblies instead of a sample profile.

wenlei added inline comments.Feb 10 2021, 2:53 PM
llvm/tools/llvm-profgen/PerfReader.h
526

Ok now I see the following error for #3, thanks for clarifying. Though I think it handles the case for hybrid sample, but if samples don't have stack, we don't emit error when there's no lbr? But that's probably fine as regular (non-hybrid) one is a TODO anyways.

'Hybrid perf sample is corrupted, No LBR sample line

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

I thought show-disassembly prints the assembly to stdout, while the generated profile goes into output file. However, if we want the tool to show assembly instead of generating profile, we may need to rename the switch to something like show-disassembly-only.

wlei updated this revision to Diff 323070.Feb 11 2021, 10:30 AM

Addressing Hongtao and Wenlei's feedback

wlei added inline comments.Feb 11 2021, 10:45 AM
llvm/tools/llvm-profgen/PerfReader.h
526

A comment showing expected input would be helpful.

Example added

For LBR part, do we want to check more than leading 0x?

Just separate a function for this and add another condition to check whether '/' exists

but if samples don't have stack, we don't emit error when there's no lbr

I think I miss this. If there is no stack and no lbr, we should emit error earlier. Added in the latest diff, thanks for the remainder.

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

I see, how about we support both?
show-disassembly print to stdout and also generate profile.
show-disassembly-only only print to output file, this can be used for the disassembly output only test.

hoy added inline comments.Feb 11 2021, 12:50 PM
llvm/tools/llvm-profgen/PerfReader.h
529

Add a test for this?

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

Sounds good, thanks!

wlei updated this revision to Diff 323369.Feb 12 2021, 9:37 AM

Address Hongtao's feedback: added test case for invalid script check

wlei added inline comments.Feb 12 2021, 9:53 AM
llvm/tools/llvm-profgen/PerfReader.h
529

Just added the invalid-perfscript.test, is that the test you want?
I think we already have the test for hybrid sample since all the profile generation test use hybrid sample as input.

wenlei added inline comments.Feb 12 2021, 10:34 AM
llvm/tools/llvm-profgen/PerfReader.h
61–62

typo: PERF_INVALID. Can we keep the enum 0 based?

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

These wrappers and layers in reader and binary feels like we're doing a bit too much for showAssembly. In the end, showAssembly is not the main purpose of the tool. How about simplify it:

  • Let's always print showAssembly to stdout. This is more like a verbose log rather than output, so I think it's better to avoid dumping it in the output file.
  • Only have show-disassembly-only mode (which is mostly used in combination with probe annotation), add early exit for that, and produce empty output profile.
wlei updated this revision to Diff 323404.Feb 12 2021, 11:02 AM

address reviewers' feedback:
rename showAssembly to show-disassembly-only and change back to print to stdout

llvm/tools/llvm-profgen/PerfReader.h
61–62

Good catch!

llvm/tools/llvm-profgen/llvm-profgen.cpp
51

thanks for the discussing, fixed!

wlei updated this revision to Diff 323408.Feb 12 2021, 11:10 AM

fix clang-tidy

wenlei added inline comments.Feb 12 2021, 11:12 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
87

Why do we need to go through PerfReader::getOutputFilename in order to access the output file from command line?

Same for ProfiledBinary::isShowDisassemblyOnlyEnabled etc., can we access the global directly (add extern declaration if needed)? These wrappers seem a bit overkill given they're not really per-binary for per-reader config.

hoy added inline comments.Feb 12 2021, 11:51 AM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
22

Why need this? Will std::out work?

llvm/tools/llvm-profgen/ProfiledBinary.h
261 ↗(On Diff #323408)

Remove this since we are keeping only show-disassembly-only?

wlei added inline comments.Feb 12 2021, 11:59 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
87

StringRef OutputFilename = PerfReader::getOutputFilename();

yeah, I miss to remove this.

can we access the global directly (add extern declaration if needed)?

I thought to use static function wrapper as the global. Yeah, use an extern global is clearer. Let me change it

wlei updated this revision to Diff 323450.Feb 12 2021, 12:33 PM

address Wenlei and Hongtao's feedback: switch to use global command line variable and some refactoring work

wlei added inline comments.Feb 12 2021, 12:35 PM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
22

This is redundant, removed.

llvm/tools/llvm-profgen/ProfiledBinary.h
261 ↗(On Diff #323408)

Yeah, overlook this, thanks for the reminder.

wenlei added inline comments.Feb 12 2021, 12:39 PM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
31

This works, but we could make it simpler:

ProfileBinary.cpp:

cl::opt<bool> ShowDisassembly("show-disassembly", cl::ReallyHidden,
                                     cl::init(false), cl::ZeroOrMore,
                                     cl::desc("Print disassembled code."));

llvm-profgen.cpp:

extern cl::opt<bool> ShowDisassembly;
wlei updated this revision to Diff 323454.Feb 12 2021, 12:52 PM

change to extern cl::opt<bool> ShowDisassemblyOnly

wlei added inline comments.Feb 12 2021, 12:55 PM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
31

Yeah, learned it!

wenlei accepted this revision.Feb 12 2021, 12:58 PM

lgtm, thanks.

This revision is now accepted and ready to land.Feb 12 2021, 12:58 PM
hoy accepted this revision.Feb 12 2021, 1:02 PM

LGTM. Maybe update the summary to reflect the command line change? Thanks.

Harbormaster completed remote builds in B89034: Diff 323408.
wlei edited the summary of this revision. (Show Details)Feb 12 2021, 1:16 PM
This revision was landed with ongoing or failed builds.Feb 12 2021, 3:19 PM
This revision was automatically updated to reflect the committed changes.