This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] New hierarchical discriminator for FS SampleFDO (ProfileData part)
ClosedPublic

Authored by xur on May 24 2021, 12:34 PM.

Details

Summary

This patch was split from https://reviews.llvm.org/D102246
[SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO
This is mainly for ProfileData part of change. It will load
FS Profile when such profile is detected. For an extbinary format profile,
create_llvm_prof tool will add a flag to profile summary section.
For other format profiles, the users need to use an internal option
(-profile-isfs) to tell the compiler that the profile uses FS discriminators.

This patch also simplified the bit API used by FS discriminators.

Diff Detail

Event Timeline

xur created this revision.May 24 2021, 12:34 PM
xur requested review of this revision.May 24 2021, 12:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 12:34 PM
wmi added inline comments.May 25 2021, 11:03 AM
llvm/include/llvm/Support/Discriminator.h
55

Can we use unsigned int instead so no implicit conversion when FSPassHighBit is used? And how about using std::array so we can use FSPassHighBit.at(i) in FSPassBitBegin and FSPassBitEnd which will have array bounds check.

109–111

Nit: For N==31, (1U << (N + 1)) - 1 is 0xFFFFFFFF so no if (N == 31) is needed.

llvm/lib/ProfileData/SampleProfReader.cpp
312–314

Nit: Discrriminator &= getDiscriminatorMask();

525–526

Nit: DiscrriminatorVal &= getDiscriminatorMask();

562–563

Nit: DiscrriminatorVal &= getDiscriminatorMask();

xur marked 3 inline comments as done.May 25 2021, 2:18 PM
xur added inline comments.
llvm/include/llvm/Support/Discriminator.h
55

OK. This sounds good to me. Will change.

109–111

Yes. using 1U is a good idea. I will change.

xur updated this revision to Diff 347790.May 25 2021, 2:42 PM

Thank Wei for the suggestions. Here is the new patch integrated his review.

hoy added inline comments.May 25 2021, 11:20 PM
llvm/include/llvm/Support/Discriminator.h
63

Does it make sense to include 0 in the FSPassHighBit array so that the check can be omitted?

How about add a assert of I <= getNumFSPasses() ?

llvm/lib/ProfileData/SampleProfReader.cpp
312

Assuming the sample loader runs in multiple places of the pipeline, does it make sense to assert the length of Discriminator matches the length of the mask? IIUC, if the number of valid bits of Discriminator is more than that of the mask, it means we are loading the profile of an earlier stage and we should use a longer mask?

wenlei added inline comments.May 26 2021, 9:34 AM
llvm/include/llvm/ProfileData/SampleProfReader.h
355

This is an API that client of profile reader will need to call, so I'm wondering if we can make it higher level - i.e, instead of requiring the client to be aware and pass in the bit-wise mask, just let client set it for base discriminator (i == 0) or i-th FS discriminator?

llvm/include/llvm/Support/Discriminator.h
55

Not a big deal but do we actually need a map here? Could just use getBaseDiscriminatorBits + i * FSDiscriminatorWidth in a helper?

88–91

There're mentions of FS passes and FD passes in multiple places in the comment, are they referring to the same thing?

llvm/lib/CodeGen/TargetPassConfig.cpp
1181–1185

A comment explaining the purpose of final discriminator pass would be helpful here.

llvm/lib/ProfileData/SampleProfReader.cpp
561

We could move the ProfileIsFS check into getDiscriminatorMask and make the mask all 1s for non-FS case?

xur marked an inline comment as done.May 26 2021, 5:52 PM

Thanks to Hongtao and Weilei for the review!

llvm/include/llvm/Support/Discriminator.h
55

Yes. I think we can do that. The reason we have a map for the bits is because I used variable length of bit for different FS discriminator passes in my earlier implementation /experiments.
I think we will end up with using fixed length here. I'll change this.

63

This part is changed to go with Wenlei's suggestion.
I will assert that I >0 and I<=0 getNumFSPasses(). This way we don't need the check. We don't have a caller with 0 as the argument anyway.

88–91

Sorry. FD is a typo. Fixed.

llvm/lib/CodeGen/TargetPassConfig.cpp
1181–1185

Will do.

llvm/lib/ProfileData/SampleProfReader.cpp
312

I'm not sure if I understand the question.
Hers the "discriminator" is the one from profile which can have all the discriminator bits (i.e. including the later passes). Here we use the mask to remove the later bits and merge them together.

561

Done.

xur updated this revision to Diff 348136.May 26 2021, 5:56 PM
xur marked an inline comment as done.

Integrated review comments from Hongtao and Weilei.

@Wei: I had to add back the special check for N==31 in getN1Bits().
My stock g++, gcc version 10.2.1 20210110 (Debian 10.2.1-6+build2)
folds "(1U << (N + 1)) - 1" to 0.

I think this is a gcc bug.

With this code check, the test case will break.

hoy accepted this revision.May 26 2021, 9:40 PM
hoy added inline comments.
llvm/include/llvm/Support/Discriminator.h
17

Nit: this is not needed anymore.

llvm/lib/ProfileData/SampleProfReader.cpp
312

You are right. I was confused.

This revision is now accepted and ready to land.May 26 2021, 9:40 PM
xur updated this revision to Diff 348308.May 27 2021, 9:30 AM
xur marked an inline comment as done.

Remove unneeded head files (thanks to Hongtao).

wmi accepted this revision.May 27 2021, 9:45 AM

LGTM.

wenlei accepted this revision.May 27 2021, 3:09 PM

lgtm, thanks.

llvm/include/llvm/ProfileData/SampleProfReader.h
357

maybe worth making a enum for FS-Passes. So it's obviously how we organize FS passes, and we don't need to pass int literals around.

xur updated this revision to Diff 348444.May 27 2021, 10:00 PM

Take Wenlei's suggestion to use enum (instead of using integer literal).
The change is bigger than I initially thought. So probably needs another look from the reviewers.

This revision was landed with ongoing or failed builds.Jun 2 2021, 10:33 AM
This revision was automatically updated to reflect the committed changes.