This is an archive of the discontinued LLVM Phabricator instance.

Use the inlinehint-threshold for hot callees.
ClosedPublic

Authored by eraman on Dec 4 2015, 1:30 PM.

Details

Summary

This patch uses the higher inlinehint-threshold for callees that are considered hot. The frontend already uses the same hotness threshold (callee's entry count >= 30% of max function entry count) to set inline hints on such callees, which results in the inlinehint-threshold being used anyway, so there is no functional change in this patch. The idea is to remove that code that sets inline hints based on hotness from the frontend and make the PGO based inlining decisions in the inliner itself. Eventually what we really want is callsite based hotness thresholds, but that is a heuristic change that needs tuning and will be done in a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 41932.Dec 4 2015, 1:30 PM
eraman retitled this revision from to Use a higher inlining threshold for hot callees..
eraman updated this object.
eraman added reviewers: davidxl, bogner.
eraman set the repository for this revision to rL LLVM.
eraman added subscribers: dnovillo, llvm-commits.
davidxl edited edge metadata.Dec 4 2015, 2:04 PM

The title of the patch is wrong. It is not setting higher threshold but moving (part of it) the FE Inline hint logic to the inliner.

The clean up looks ok to me (NFC at this point). I added Duncan to the list to see if there are any concerns.

eraman retitled this revision from Use a higher inlining threshold for hot callees. to Use the inlinehint-threshold for hot callees..Dec 4 2015, 2:23 PM
eraman updated this object.
eraman added a subscriber: eraman.Dec 4 2015, 2:24 PM

Well, it does use a higher (when compared to the default) threshold, but
I've changed the title and description to make it more clear.

davidxl accepted this revision.Dec 7 2015, 10:43 AM
davidxl edited edge metadata.
This revision is now accepted and ready to land.Dec 7 2015, 10:43 AM

Once this is in, please update lib/Transforms/IPO/SampleProfile.cpp. It does something similar.

Also, should cold functions be marked with the cold hint?

LGTM. As Justin says, please continue the clean up effort: 1) unify the profile summary data for PGO and Sample FDO 2) move the cold attribute logic to LLVM

eraman updated this revision to Diff 43180.Dec 17 2015, 1:51 PM
eraman edited edge metadata.
  1. Use the hint threshold only if it is greater than regular threshold.
  2. Added a test case.

Duncan and Justin, does this patch look good?

eraman updated this revision to Diff 43282.Dec 18 2015, 4:55 PM

I am now checking for cold calls and using the cold threshold as well and added a test case.

Earlier in this thread I had commented that Cold attribute is used in other places as well, but I was wrong. Branch probability computation checks for Cold attribute in instruction and not the function. With this change, both setting of cold and inlinehint attribute can be removed from CodeGenPGO.cpp in CFE. Does this look good?

This looks very good to me (also addressed cold attribute feedback by Justin) -- and the test cases are nice. Ok to check in with the 'nits' fixed. Looking forward to the FE cleanup.

lib/Transforms/IPO/Inliner.cpp
319

I would suggest parametize 0.3 and 0.01 below, but since there is on-going work on profile summary, it is probably not necessary.

test/Transforms/Inline/inline-cold-callee.ll
36 ↗(On Diff #43282)

nit -- please make MaxFunctionCount larger so that the test case is more stable to future tunings.

I'll address David's comment on test case (although I don't think it matters much) and if I don't hear from Justin and Duncan will submit this patch later today.

This revision was automatically updated to reflect the committed changes.