This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Update default cutoff threshold for machine function splitter.
ClosedPublic

Authored by snehasish on Oct 8 2020, 5:47 PM.

Details

Summary

Based on internal testing at Google we found that setting the profile
summary cutoff threshold to 999950 yields the best results in terms of
itlb and icache metrics (as observed on Intel CPUs).

*default* = Split out code if no profile count available for block
*size-%* = The fraction of bytes split out of .text and .text.hot
*itlb* = Misses per kilo instructions (MPKI) for itlb
*icache* = Misses per kilo instructions (MPKI) for L1 icache

Search1

cutoffsize-%itlbicache
default42.58610.08221512.46363
99999944.93500.07671942.44416
99995050.06600.0757442.4091
99950056.91580.0825642.4188
99500063.86250.08149272.42832
99000071.73140.1069062.57785

Search2

cutoffsize-%itlbicache
default2.88450.6267124.73245
9999993.32910.6023094.70045
9999503.85770.5878424.71632
9995004.41700.635774.68351
9950005.10200.6579694.82272
9900005.71530.7191225.39496

Diff Detail

Event Timeline

snehasish created this revision.Oct 8 2020, 5:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 5:47 PM
snehasish requested review of this revision.Oct 8 2020, 5:47 PM

Arguably since this is going to be very cpu dependent perhaps it should be
part of TTI?

-eric

snehasish updated this revision to Diff 297095.Oct 8 2020, 6:14 PM

Add a FIXME to move defaults to TTI.

This pass is only enabled on X86 platforms (D87047 for clang options and platform check). Longer term it does make sense to move it to TTI so I've added a FIXME to get this change off the critical path and I'll follow up with a refactoring change.

MaskRay added a subscriber: MaskRay.Oct 8 2020, 6:18 PM
MaskRay added inline comments.
llvm/lib/CodeGen/MachineFunctionSplitter.cpp
42

Nit: TTI -> TargetTransformInfo

snehasish updated this revision to Diff 297104.Oct 8 2020, 6:32 PM

Spell out TTI as TargetTransformInfo, rebase.

snehasish marked an inline comment as done.Oct 8 2020, 6:32 PM

PTAL, thanks!

tmsriram accepted this revision.Oct 13 2020, 9:55 PM

LGTM. The concerns raised have been addressed and it will be moved to TTI while porting to other platforms.

This revision is now accepted and ready to land.Oct 13 2020, 9:55 PM

Sorry, didn't see this last time. Couple of inline comments, mostly nits.

-eric

llvm/lib/CodeGen/MachineFunctionSplitter.cpp
43

Can you comment the value? What it's based off of, what it means, etc.

Add comments, rebase.

snehasish marked an inline comment as done.Oct 13 2020, 11:10 PM

Thanks for the review.

echristo accepted this revision.Oct 14 2020, 10:43 AM

Awesome. Thanks for the update :)

-eric