Page MenuHomePhabricator

[SampleFDO] New hierarchical discriminator for FS SampleFDO (llvm-profdata part)
ClosedPublic

Authored by xur on Jun 2 2021, 12:23 PM.

Details

Summary

This patch was split from https://reviews.llvm.org/D102246
[SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO
This is for llvm-profdata part of change. It sets the bit masks for the
profile reader in llvm-profdata. Also add an internal option
"-fs-discriminator-pass" for show and merge command to process the profile
offline.

Diff Detail

Event Timeline

xur requested review of this revision.Jun 2 2021, 12:23 PM
xur created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 12:23 PM
wmi added inline comments.Jun 2 2021, 5:21 PM
llvm/tools/llvm-profdata/llvm-profdata.cpp
942–943

Not sure if it is better to use cl::opt<enum>. Maybe not. But it may still be better to mention the input value correspond to the enum value of llvm::sampleprof::FSDiscriminatorPass.

945–949

May be shared with the corresponding part in show_main.

xur added inline comments.Jun 3 2021, 11:13 AM
llvm/tools/llvm-profdata/llvm-profdata.cpp
942–943

Using enum, we don't need to code to check the range. Let me change to use enum.

945–949

OK. I will move the option definition to a file scope static.

xur updated this revision to Diff 349619.Jun 3 2021, 11:22 AM

Per Wei's suggestion, using enum option.

wmi accepted this revision.Jun 3 2021, 5:09 PM

LGTM.

This revision is now accepted and ready to land.Jun 3 2021, 5:09 PM
wenlei added inline comments.Jun 3 2021, 5:24 PM
llvm/include/llvm/Support/Discriminator.h
59

The cl::opt FSDiscriminatorPassOption doesn't have Pass0. Was it intentional to allow Pass0 as alias for Base?

hoy added inline comments.Jun 3 2021, 5:26 PM
llvm/tools/llvm-profdata/llvm-profdata.cpp
468

Nit: add a blank line above

570

I'm wondering if this can be moved into SampleProfileReader::create with the mask as an extra parameter of it.

xur marked an inline comment as done.Jun 4 2021, 9:21 AM
xur added inline comments.
llvm/include/llvm/Support/Discriminator.h
59

Yes. that's an alias (same as Pass4 and PassLast).
I did not add the aliases the cl::opt because they are not essential part of the functionality (this is an internal optional anyway).

llvm/tools/llvm-profdata/llvm-profdata.cpp
570

That's a good idea -- I think this would simplify the code. Let me do the change.

xur updated this revision to Diff 349906.Jun 4 2021, 10:04 AM

Integrated Hongtao's suggestion to move setDiscriminatorMaskedBitFrom() to SampleProfileReader::create()

hoy accepted this revision.Jun 4 2021, 10:35 AM
hoy added inline comments.
llvm/tools/llvm-profdata/llvm-profdata.cpp
570

Thanks for working on it. LGTM.