Page MenuHomePhabricator

[Frontend] 'Show hotness' can be used with a sampling profile
ClosedPublic

Authored by modocache on Jun 10 2017, 10:35 AM.

Details

Summary

Prior to this change, using -fdiagnostics-show-hotness with a sampling
profile specified via -fprofile-sample-use= would result in the Clang
frontend emitting a warning: "argument '-fdiagnostics-show-hotness' requires
profile-guided optimization information". Of course, a sampling profile
*is* profile-guided optimization information, so the warning is misleading.
Furthermore, despite the warning, hotness was displayed based on the data in
the sampling profile.

Prevent the warning from being emitted when a sampling profile is used, and
add a test that verifies this.

Event Timeline

modocache created this revision.Jun 10 2017, 10:35 AM
modocache added inline comments.Jun 10 2017, 10:40 AM
test/Frontend/optimization-remark-with-hotness.c
37

Ideally, I'd like for this test to be identical to the ones that use -verify above, save for using -fprofile-sample-use. However I couldn't figure out how to write optimization-remark-with-hotness-sample.proftext such that it would produce hotness info for each of the functions. I'm able to confirm, using real sampling from another program of mine, that remarks using AutoFDO data do indeed show hotness, but this test does not verify this in its current state.

Any advice here? I wrote optimization-remark-with-hotness-sample.proftext based on the format specified in https://clang.llvm.org/docs/UsersManual.html#sample-profile-text-format, but maybe it's missing something that would allow hotness to be displayed?

anemet added inline comments.Jun 10 2017, 11:01 AM
test/Frontend/optimization-remark-with-hotness.c
2–35

Why don't you put the sampling test right under the instrumented test. Also I don't think we want a separate --check-prefix for it. It should just use the default CHECK since the entire point is that the two should be identical. In other words, please check that we don't warn on either case.

anemet added inline comments.Jun 10 2017, 11:24 AM
test/Frontend/optimization-remark-with-hotness.c
37

I don't have any immediate ideas since I am not familiar with sample-based profiling unfortunately. I will study this later unless you beat me to it or David has some ideas.

davidxl added inline comments.Jun 10 2017, 11:43 AM
test/Frontend/optimization-remark-with-hotness.c
37

The hotness is based on profile summary, so you need to adjust the bar's entry count and inline instance of foo's count to be very large value to let compiler decide it is hot.

modocache updated this revision to Diff 102153.Jun 11 2017, 7:24 PM

Thanks for the suggestions! I moved the sampling test close to the instrumented one, and adjusted bar and foo entry count in the hopes og getting the remarks to include hotness. No luck, however -- the test currently fails. I will look deeper into how hotness and sampling profiles work.

modocache retitled this revision from [Frontend 'Show hotness' can be used with a sampling profile to [Frontend] 'Show hotness' can be used with a sampling profile.Jun 12 2017, 7:10 PM
This revision is now accepted and ready to land.Jun 13 2017, 10:43 AM
modocache updated this revision to Diff 103647.Jun 22 2017, 2:44 PM

Update the sampling profile text so that it produces the hotness expected by the test. This is ready to go :)

modocache marked 4 inline comments as done.Jun 22 2017, 2:47 PM
modocache closed this revision.Jun 22 2017, 7:39 PM
anemet edited edge metadata.Dec 1 2017, 10:31 AM
anemet added a subscriber: davide.

@modocache, @davide, are you guys sure this feature is working? The test does not actually check whether hotness is included in the remarks and when I run it manually they are missing. In D40678, I am filtering out remarks with no hotness when any threshold is set all the remarks are filtered out in this new test.

So either the test is incorrect or somehow with sample-based profiling we don't get hotness info.

Any ideas? I am inclined to just remove this test for now and file a bug to fix this in order to unblock D40678.

@modocache, @davide, are you guys sure this feature is working? The test does not actually check whether hotness is included in the remarks and when I run it manually they are missing. In D40678, I am filtering out remarks with no hotness when any threshold is set all the remarks are filtered out in this new test.

So either the test is incorrect or somehow with sample-based profiling we don't get hotness info.

Any ideas? I am inclined to just remove this test for now and file a bug to fix this in order to unblock D40678.

I don't know, I haven't reviewed this feature, but I can take a look later today.

Looks like it's a test problem. When I tweak the sample profile file according to https://clang.llvm.org/docs/UsersManual.html#sample-profile-text-format, I do get hotness on the remarks.

Sorted these out in rL319576, rL319577 and rL319578.