- Notifications
You must be signed in to change notification settings - Fork5.2k
JIT: preliminary version of profile-based inline policy#44427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Add a new inline policy that can be used when a method has profile data.It uses the call site frequency to boost profitability. Size and per-callbenefit are currently using the estimates done by the model policy.Not on by default. Set COMPlus_JitInlinePolicyProfile=1 to enable.Add testing to weekly experimental runs.
AndyAyersMS commentedNov 9, 2020
cc @dotnet/jit-contrib |
EgorBo commentedNov 9, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So it fixes#41923 (I've just checked) 🎉 Also, is it expected that it's now able to inline methods with more than stringFoo()=>"hello";[MethodImpl(MethodImplOptions.NoInlining)]boolDoWork(){returnFoo()=="hello";} Currently, inliner gives up here because G_M53056_IG01:subrsp,40;; bbWeight=1 PerfScore 0.25G_M53056_IG02:movrcx,0xD1FFAB1Emovrdx, gword ptr[rcx]movrcx,rdxcall System.String:Equals(System.String,System.String):boolnop;; bbWeight=1 PerfScore 3.75G_M53056_IG03:addrsp,40ret Your branch (with profile): G_M53056_IG01: ;; offset=0000H ;; bbWeight=1 PerfScore 0.00G_M53056_IG02: ;; offset=0000H B801000000moveax,1 ;; bbWeight=1 PerfScore 0.25G_M53056_IG03: ;; offset=0005H C3ret (basically, fixes#33338) |
BruceForstall left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One fix needed in config value string, otherwise LGTM
src/coreclr/src/jit/inlinepolicy.h Outdated
| classProfilePolicy :publicDiscretionaryPolicy | ||
| { | ||
| public: | ||
| // Construct a ModelPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: copy-paste comment
| CONFIG_INTEGER(JitInlinePolicyModel,W("JitInlinePolicyModel"),0) | ||
| CONFIG_INTEGER(JitInlinePolicyProfile,W("JitInlinePolicyProfile"),0) | ||
| CONFIG_INTEGER(JitInlinePolicyProfileThreshold,W("JitInlinePolicyProfile"),40) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
String is wrong here: JitInlinePolicyProfile => JitInlinePolicyProfileThreshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for spotting this.
AndyAyersMS commentedNov 10, 2020
Yes. I'm aiming to have very few arbitrary early limits on what might be inlined, to let the profile data and heuristics determine what's best. We will need to have some limit on the max IL size we'll consider so that we don't spend time analyzing large methods that can't possibly be good inlines. Not sure what the right value for that limit is just yet; I have it set to 1000 bytes here. |
AndyAyersMS commentedNov 11, 2020
Failure seems unrelated; none of this code should be enabled by default. |
Add a new inline policy that can be used when a method has profile data.
It uses the call site frequency to boost profitability. Size and per-call
benefit are currently using the estimates done by the model policy.
Not on by default. Set COMPlus_JitInlinePolicyProfile=1 to enable.
Add testing to weekly experimental runs.