-
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
Pass exact context to getMethodInfo #88025
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBasically, it's Andy's patch that he never landed and asked me to. Closes #88021 bool Test() => EqualityComparer<string>.Default.Equals("str", "str"); New codegen: ; Method Prog:Foo():bool:this
mov eax, 1
ret
; Total bytes of code: 6
|
Wow I got some impressive performance improvements in my local benchmark. But there's still some weird codegen which is unnecessary. For example: bool Test() => new Foo(1, "hello", "test") == new Foo(1, "test", "hello");
record struct Foo(int x, string y, string z); The codegen for G_M50849_IG01: ;; offset=0000H
;; size=0 bbWeight=1.00 PerfScore 0.00
G_M50849_IG02: ;; offset=0000H
xor eax, eax
je SHORT G_M50849_IG04
;; size=4 bbWeight=1.00 PerfScore 1.25
G_M50849_IG03: ;; offset=0004H
xor eax, eax
jmp SHORT G_M50849_IG05
;; size=4 bbWeight=0.50 PerfScore 1.12
G_M50849_IG04: ;; offset=0008H
xor eax, eax
;; size=2 bbWeight=0.50 PerfScore 0.12
G_M50849_IG05: ;; offset=000AH
ret
;; size=1 bbWeight=1 PerfScore 1.00 which is effectively equivalent to: xor eax, eax
ret |
/azp run runtime-coreclr pgo |
Azure Pipelines successfully started running 1 pipeline(s). |
…tmethodinfo # Conflicts: # src/coreclr/inc/jiteeversionguid.h
7cf5c9e
to
3192c83
Compare
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress |
Azure Pipelines successfully started running 4 pipeline(s). |
@AndyAyersMS @jakobbotsch PTAL JIT side - it should be ready now, CI failures are not related |
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
} | ||
else if (ctx is InstantiatedType instantiatedCtxType) | ||
{ | ||
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType( |
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.
This won't work correctly for non-shared generic methods on shared generic types.
For example, G<string>.M<int>
. GetTypicalMethodDefinition
will drop the method instantiation, so we will end up with G<string>.M<T>
. I am not sure whether this problem is reachable in practice currently.
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.
I've just pushed a change:
MethodDesc methodTypicalDef = method.GetTypicalMethodDefinition();
if (methodTypicalDef.RequiresInstMethodDescArg())
{
// Presumably, a rare case: we had MyClass<T1>.Foo<int>() method and
// GetTypicalMethodDefinition() converted it to MyClass<T1>.Foo<T2>
}
else
{
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(methodTypicalDef, instantiatedCtxType);
}
is it enough?
Co-authored-by: Jan Kotas <[email protected]>
…e-1 into pass-context-getmethodinfo
72b2ec5
to
08ac385
Compare
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
This crash looks related to the change: https://helix.dot.net/api/2019-06-17/jobs/47d2c98f-aa2e-4f5c-a38b-b6fc165d4b0b/workitems/System.Runtime.CompilerServices.Unsafe.Tests/console |
Looks like it is not since it can be found in other runs e.g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=327970&view=ms.vss-test-web.build-test-results-tab&runId=6761902&resultId=150352&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab - I'll file an issue (#88339) (PS: would love NativeAOT Libraries tests to have its own pipeline to avoid running Mono workloads) |
Co-authored-by: Jan Kotas <[email protected]>
@jkotas anything else to be done here? |
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.
LGTM!
@EgorBo's dotnet#88025 made it possible to undo conservative scanning logic added in dotnet/corert#7618. Saves 0.4% in size for Stage1 and Stage2 apps.
@EgorBo's #88025 made it possible to undo conservative scanning logic added in dotnet/corert#7618. Saves 0.4% in size for Stage1 and Stage2 apps.
Basically, it's Andy's patch that he never landed and asked me to (with some modifications).
Closes #88021
New codegen:
It is also needed for #87847