[WIP] conditionally assign (exact)contextHandle #45321
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
|
||
| // Update context handle. | ||
| if ((exactContextHandle != nullptr) && (*exactContextHandle != nullptr)) | ||
| { | ||
| *exactContextHandle = MAKE_METHODCONTEXT(derivedMethod); |
There was a problem hiding this comment.
Why it didn't work that way:
info.compCompHnd->getClassName(objClass): "G`1[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]"
info.compCompHnd->getClassName(baseClass): "G`1[__Canon]"
info.compCompHnd->getClassName(derivedClass): "G`1[__Canon]"
|
Here is the failing assert: runtime/src/coreclr/src/vm/jitinterface.cpp Line 1003 in c1b5ac0 |
|
Fun fact: Checked run with removed runtime/src/coreclr/src/vm/jitinterface.cpp Line 1003 in c1b5ac0 produces only single failure: EDIT |
|
@Rattenkrieg I suspect fixing this also requires runtime changes. Ignoring generic methods for the moment, I think the context must always be set to the derived class, but currently the jit does not have the exact version. You can get away with using the object class times (since often the derived and object class are the same), but I suspect this won't always work. Interesting test cases are those where we shift from generic to non-generic (and vice versa) as we move up the hierarchy. |
|
@jkotas @Rattenkrieg I think this is a plausible fix: (SPMI/Crossgen2 handling not fully there yet) We were losing the exactness because We won't see generic virtual methods in It handles cases like the following: using System;
using System.Runtime.CompilerServices;
class B
{
public virtual int F() => 33;
}
class D<T> : B
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override int F() => typeof(T) == typeof(string) ? 44 : 55;
}
class E : D<string>
{
}
class G<T> : E
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override int F() => typeof(T) == typeof(string) ? 66 : 77;
}
class Program
{
public static void Main()
{
B b = new B();
D<string> ds = new D<string>();
E e = new E();
G<string> gs = new G<string>();
Console.WriteLine($"B.F() = {b.F()}");
Console.WriteLine($"D<string>.F() = {ds.F()}");
Console.WriteLine($"E.F() = {e.F()}");
Console.WriteLine($"G<string>.F() = {gs.F()}");
}
}But it does not seem to crop up all that often. PMI only shows 19 diffs out of 259k methods. Many of those just replace one level of call with another, though there is a good diff in Roslyn's where we go from G_M5034_IG17:
cmp gword ptr [rdi+264], 0
je SHORT G_M5034_IG20
;; bbWeight=1 PerfScore 3.00
G_M5034_IG18:
mov rcx, gword ptr [rdi+264]
mov rax, 0xD1FFAB1E
cmp dword ptr [rcx], ecx
call qword ptr [rax]EmbeddedTypesManager`21:get_IsFrozen():bool:this
test eax, eax
je SHORT G_M5034_IG20
mov rcx, gword ptr [rdi+264]
mov rdx, rsi
mov r8, rbx
mov rax, 0xD1FFAB1E
mov rax, qword ptr [rax]
cmp dword ptr [rcx], ecxto G_M5034_IG17:
mov rcx, gword ptr [rdi+264]
test rcx, rcx
je SHORT G_M5034_IG20
;; bbWeight=1 PerfScore 3.25
G_M5034_IG18:
mov rdx, rcx
cmp dword ptr [rdx], edx
add rdx, 56
cmp gword ptr [rdx], 0
je SHORT G_M5034_IG20
mov rdx, rsi
mov r8, rbx
mov rax, 0xD1FFAB1E
mov rax, qword ptr [rax]
cmp dword ptr [rcx], ecx |
|
@Rattenkrieg I am going to rework my changes to use the same kind of struct you were considering ... also rebasing on top of #45044 to simplify some of the interface plumbing. |
|
@AndyAyersMS sure! Since I cannot dedicate enough time to hack runtime on any other day than weekends I don't want to block you in case you willing to work on this issue at the moment or just have a better idea. I can close this PR in favor of your branch. |
|
Closing in favor of #45526 |
ghost
locked as resolved and limited conversation to collaborators
Fixes #38477
I hacked this in a straightforward manner while I was working on #43668 but I was in doubt that such dumb fix is enough until I saw @AndyAyersMS 's comment
I've ended with similar asm but withSee EDITSystem.Object:.ctor()call not eliminated:GivenHowever non-virtualFAILED: noinline per VMI wonder if @AndyAyersMS was testing this on non-master branch.public bool M() => typeof(T) == typeof(string);jits toSo there is definitely an opportunity for me to hack further.
I have issues with crafting jit diffs atm, will submit it soon.
EDIT:
Above asm was produced with debug build where
JIT_FLAG_DEBUG_CODEpreventsSystem.Object:.ctor()inlining.With checked build I have asm similar to @AndyAyersMS