-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: preliminary version of profile-based inline policy #44427
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-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.
cc @dotnet/jit-contrib |
So it fixes #41923 (I've just checked) 🎉 Also, is it expected that it's now able to inline methods with more than string Foo() => "hello";
[MethodImpl(MethodImplOptions.NoInlining)]
bool DoWork()
{
return Foo() == "hello";
} Currently, inliner gives up here because G_M53056_IG01:
sub rsp, 40
;; bbWeight=1 PerfScore 0.25
G_M53056_IG02:
mov rcx, 0xD1FFAB1E
mov rdx, gword ptr [rcx]
mov rcx, rdx
call System.String:Equals(System.String,System.String):bool
nop
;; bbWeight=1 PerfScore 3.75
G_M53056_IG03:
add rsp, 40
ret Your branch (with profile): G_M53056_IG01: ;; offset=0000H
;; bbWeight=1 PerfScore 0.00
G_M53056_IG02: ;; offset=0000H
B801000000 mov eax, 1
;; bbWeight=1 PerfScore 0.25
G_M53056_IG03: ;; offset=0005H
C3 ret (basically, fixes #33338) |
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
class ProfilePolicy : public DiscretionaryPolicy | ||
{ | ||
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
@@ -402,6 +402,8 @@ CONFIG_STRING(JitInlineReplayFile, W("JitInlineReplayFile")) | |||
#endif // defined(DEBUG) || defined(INLINE_DATA) | |||
|
|||
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.
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. |
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.