-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Devirtualization and inlining for GVM #112353
base: main
Are you sure you want to change the base?
Conversation
Looks like CI failures are related? also, the diffs look more like inliner actually no longer inlines what it used to? |
They should be fixed with 4b809c8, where we must not allow a method
Not sure about this for now, may need more investigation. Let's see the latest run. |
Seems that the VM can dispatch a generic method if there's no method |
Devirtualizing GVM without inlining is supported now. |
No spmi diffs and only 1 method affected in jit-diffs. Can't tell it's a big improvement for that method, but it definitely implies a poor test coverage for this optimization. Since changes around method resolving are always risky - do we want to proceed? Is there a real-world motivation behind this? |
I think it's in a great form now, while seems that it still has some bad interactions with boxed structs during inlining, need to investigate... |
Seems that lowering on windows-x86 is unhappy about not spilling the
|
@jakobbotsch can you trigger a pri1/outerloop and pgostress run on this PR? I would like to verify my prototype doesn't break anything so that I can move forward with a polished implementation. |
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop |
Azure Pipelines successfully started running 3 pipeline(s). |
coreclr outerloop failures are unrelated (as there's no GVM at all). |
Minimal repro: ParallelQuery<int> query = Array.Empty<int>().AsParallel();
var cast1 = query.Cast<string>();
var cast2 = cast1.Cast<int>();
Use(cast2);
[MethodImpl(MethodImplOptions.NoInlining)]
static void Use<T>(T obj) { } It will throw
The tree after inlining is
I don't see any problem with it. If I insert a no-op between ParallelQuery<int> query = Array.Empty<int>().AsParallel();
var cast1 = query.Cast<string>();
Use(42);
var cast2 = cast1.Cast<int>();
Use(cast2); The tree this time after inlining is:
I don't see there's any subtle difference between the two trees, so I'm puzzled. |
The generic context for the second - [000049] H---------- gctx \--* CNS_INT(h) long 0x7ff830c80748 method System.Linq.ParallelQuery`1[System.__Canon]:Cast[int]():System.Linq.ParallelQuery`1[int]:this
+ [000052] H---------- gctx \--* CNS_INT(h) long 0x7ff827f10898 method System.Linq.ParallelQuery`1[System.String]:Cast[int]():System.Linq.ParallelQuery`1[int]:this |
Yeah I found it out right after you post the observation :) The codegen diff is https://www.diffchecker.com/6hQxtIfb/ Especially, - mov rcx, rbx
- mov rdx, 0x7FF830C50898 ; System.Linq.ParallelQuery`1[System.String]:Cast[int]():System.Linq.ParallelQuery`1[int]:this
+ mov rcx, rax
+ mov rdx, 0x7FF830C60748 ; System.Linq.ParallelQuery`1[System.__Canon]:Cast[int]():System.Linq.ParallelQuery`1[int]:this It seems that we somehow lost the concrete type context in some cases, the generic context being passed to |
Doesn't seem like a jit issue, instead it should be handled in VM, where we need to fetch type instantiations for the base method based on generic context. This issue can happen when the derived method table is not generic while the base method table is a shared generics. |
We use
FindOrCreateAssociatedMethodDesc
withallowInstParam=false
to get the exactMethodDesc
for a devirted GVM.While the resulted
MethodDesc
may be an instantiation stub, so eitherInstParam
that can be fetched fromWrappedMethodDesc
if it doesn't require a runtime lookupRUNTIMELOOKUP
node we created forldvirtftn
if it requires a runtime lookup (in this caseFindOrCreateAssociatedMethodDesc
will yield us a method handle that has shared generics in the method instantiation).So if we see a
RUNTIMELOOKUP
, we push it to the call args as theInstParam
, otherwise we can use the instantiated entry of theWrappedMethodDesc
as theInstParam
.Also extend the late devirt a bit to support devirting generic virtual methods.
To make sure we can get the method handle or
RUNTIMELOOKUP
, we need to stop spillingldvirtftn
. However, NAOT and lowering is not happy aboutgtCallAddr
being aCALL
, so we split the call after inlining is done.NativeAOT/R2R is not support yet (due to fat function pointers).
Example:
Before:
After:
/cc: @jakobbotsch @AndyAyersMS