This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] Add use-sample-profile function attribute
ClosedPublic

Authored by wmi on May 14 2020, 12:33 PM.

Details

Summary

When sampleFDO is enabled, people may expect they can use -fno-profile-sample-use to opt-out using sample profile for a certain file. That could be either for debugging purpose or for performance tuning purpose. However, when thinlto is enabled, if a function in file A compiled with -fno-profile-sample-use is imported to another file B compiled with -fprofile-sample-use, the inlined copy of the function in file B may still get its profile annotated.

The inconsistency may even introduce profile unused warning because if the target is not compiled with explicit debug information flag, the function in file A won't have its debug information enabled (debug information will be enabled implicitly only when -fprofile-sample-use is used). After it is imported into file B which is compiled with -fprofile-sample-use, profile annotation for the outline copy of the function will fail because the function has no debug information, and that will trigger profile unused warning.

We add a new attribute use-sample-profile to control whether a function will use its sample profile no matter for its outline or inline copies. That will make the behavior of -fno-profile-sample-use consistent.

There are some tests to fix. I will fix them after reviewers think the patch is generally ok.

Diff Detail

Event Timeline

wmi created this revision.May 14 2020, 12:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2020, 12:33 PM

If we make it a function attribute, do we need to reject pre-LTO (CGSCC) inlining between function with different attribute value? If we always expect this attribute to be consistent among functions from the same module, this won't be a problem. However having it as a function attribute does offer function level flexibility (e.g. if this is exposed through __attribute(..)__), and we may want to account for it..

If this is for experimental use, why not just introducing a LLVM internal option that specify a list of function to be skipped? It should be applied to inline instances of the function specified?

wmi added a comment.May 17 2020, 9:13 PM

If we make it a function attribute, do we need to reject pre-LTO (CGSCC) inlining between function with different attribute value? If we always expect this attribute to be consistent among functions from the same module, this won't be a problem. However having it as a function attribute does offer function level flexibility (e.g. if this is exposed through __attribute(..)__), and we may want to account for it..

Good point. Although function level flexibility is not needed so far, people may create IR with different attribute values. It is better to block the inlining between function with different attribute values to keep it consistent. I will do it this way.

wmi added a comment.May 17 2020, 9:32 PM

If this is for experimental use, why not just introducing a LLVM internal option that specify a list of function to be skipped? It should be applied to inline instances of the function specified?

One of the use case is to add -fno-profile-sample-use for some library so the library won't use profile information for any build target. If we support it using LLVM internal option then we need to enable the internal option globally. Everytime someone wants to add a function into the list, the global internal option has to be changed. That is not very convenient.

And similar as attribute support, for the function in the to-be-skipped list, it cannot be inlined into function which is not in the list, otherwise its inline instance may still be annotated with profile. function not in the list also cannot be inlined into function in the list. So the implementation won't be much simpler than attribute support.

wmi updated this revision to Diff 265132.May 19 2020, 9:49 PM

Address Wenlei's comment.

hoyFB added inline comments.May 19 2020, 10:26 PM
llvm/lib/Analysis/InlineAdvisor.cpp
321 ↗(On Diff #265132)

How about moving this into functionsHaveCompatibleAttributes in InlineCost.cpp?

wmi marked an inline comment as done.May 20 2020, 7:49 AM
wmi added inline comments.
llvm/lib/Analysis/InlineAdvisor.cpp
321 ↗(On Diff #265132)

That is a good point. I will change it. Thanks!

wmi updated this revision to Diff 265290.May 20 2020, 10:01 AM

Address Hongtao's comment.

I guess you accidentally missed InlineCost.cpp in latest version?

wmi added a comment.May 20 2020, 11:39 AM

I guess you accidentally missed InlineCost.cpp in latest version?

I don't need to change InlineCost.cpp. Adding "def : CompatRule<"isEqual<UseSampleProfileAttr>">" in include/llvm/IR/Attributes.td will automatically update the function hasCompatibleFnAttrs auto-generated. hasCompatibleFnAttrs is indirectly called by functionsHaveCompatibleAttributes and getInlineCost.

wenlei accepted this revision.May 21 2020, 11:02 AM
In D79959#2047325, @wmi wrote:

I guess you accidentally missed InlineCost.cpp in latest version?

I don't need to change InlineCost.cpp. Adding "def : CompatRule<"isEqual<UseSampleProfileAttr>">" in include/llvm/IR/Attributes.td will automatically update the function hasCompatibleFnAttrs auto-generated. hasCompatibleFnAttrs is indirectly called by functionsHaveCompatibleAttributes and getInlineCost.

Ah, thanks for clarifying! LGTM.

This revision is now accepted and ready to land.May 21 2020, 11:02 AM
MaskRay added inline comments.
clang/test/CodeGen/use-sample-profile-attr.c
5

You need one RUN: for each continuation line.

MaskRay added inline comments.May 21 2020, 11:58 AM
clang/lib/CodeGen/CodeGenFunction.cpp
794

The code self explains. I think the comment does not add much value.

llvm/test/Transforms/Inline/inline-incompat-attrs.ll
24

You may consider ;; for comments, which can make comments stand out from RUN/CHECK lines.

Perhaps you have an opinion on D79276? :)

llvm/test/Transforms/SampleProfile/use-sample-profile-attr.ll
118

trailing empty line

wmi marked 4 inline comments as done.May 29 2020, 10:19 AM
wmi added inline comments.
clang/test/CodeGen/use-sample-profile-attr.c
5

clang-format changed it to multiple lines. Thanks for catching it. Fixed.

llvm/test/Transforms/Inline/inline-incompat-attrs.ll
24

Changed.

llvm/test/Transforms/SampleProfile/use-sample-profile-attr.ll
118

Fixed.

wmi updated this revision to Diff 267285.May 29 2020, 10:20 AM

Address MaskRay's comment and fix a bunch of tests.

MaskRay added inline comments.May 29 2020, 11:36 AM
clang/test/CodeGen/use-sample-profile-attr.c
5

It may be easier to read if you indent continuation lines by 2 spaces.

wmi marked an inline comment as done.Jun 2 2020, 12:48 PM
wmi added inline comments.
clang/test/CodeGen/use-sample-profile-attr.c
5

Yes, that is better. Fixed.

wmi updated this revision to Diff 267957.Jun 2 2020, 12:48 PM

Address MaskRay's comment.

MaskRay accepted this revision.Jun 2 2020, 1:17 PM

Thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 6:08 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mtrofin added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
794

Could this be applied by a pass, early on in PassBuilder::buildPerModuleDefaultPipeline, if we determine we're not in ThinLTO post link? That way it would be a frontend-independent solution.