JIT: Fix constrained call dereferences happening too early #86638
Conversation
Constrained calls in IL include an implicit dereference that occurs as
part of the call. The JIT was adding this dereference on top of the
'this' argument tree, which makes it happen too early (before other
arguments are evaluated). This changes the importer to spill when
necessary to preserve ordering.
For:
```cil
.method private hidebysig static void Foo(class Runtime_73615/C arg) cil managed noinlining
{
// Code size 21 (0x15)
.maxstack 2
IL_0000: ldarga.s arg
IL_0002: ldarga.s arg
IL_0004: call int32 Runtime_73615::Bar(class Runtime_73615/C&)
IL_0009: constrained. Runtime_73615/C
IL_000f: callvirt instance void Runtime_73615/C::Baz(int32)
IL_0014: ret
}
```
Before:
```
***** BB01
STMT00000 ( 0x000[E-] ... 0x014 )
[000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this
[000004] n---G------ this ├──▌ IND ref
[000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0]
[000002] --C-G------ arg1 └──▌ CALL int Runtime_73615:Bar(byref):int
[000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0]
```
After:
```
***** BB01
STMT00000 ( 0x000[E-] ... 0x014 )
[000005] -AC-G------ ▌ ASG int
[000004] D------N--- ├──▌ LCL_VAR int V02 tmp1
[000002] --C-G------ └──▌ CALL int Runtime_73615:Bar(byref):int
[000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0]
***** BB01
STMT00001 ( ??? ... ??? )
[000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this
[000007] n---G------ this ├──▌ IND ref
[000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0]
[000006] ----------- arg1 └──▌ LCL_VAR int V02 tmp1
```
Fix dotnet#73615
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsConstrained calls in IL include an implicit dereference that occurs as part of the call. The JIT was adding this dereference on top of the 'this' argument tree, which makes it happen too early (before other arguments are evaluated). This changes the importer to spill when necessary to preserve ordering. For: .method private hidebysig static void Foo(class Runtime_73615/C arg) cil managed noinlining
{
// Code size 21 (0x15)
.maxstack 2
IL_0000: ldarga.s arg
IL_0002: ldarga.s arg
IL_0004: call int32 Runtime_73615::Bar(class Runtime_73615/C&)
IL_0009: constrained. Runtime_73615/C
IL_000f: callvirt instance void Runtime_73615/C::Baz(int32)
IL_0014: ret
}Before: ***** BB01
STMT00000 ( 0x000[E-] ... 0x014 )
[000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this
[000004] n---G------ this ├──▌ IND ref
[000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0]
[000002] --C-G------ arg1 └──▌ CALL int Runtime_73615:Bar(byref):int
[000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0]After: ***** BB01
STMT00000 ( 0x000[E-] ... 0x014 )
[000005] -AC-G------ ▌ ASG int
[000004] D------N--- ├──▌ LCL_VAR int V02 tmp1
[000002] --C-G------ └──▌ CALL int Runtime_73615:Bar(byref):int
[000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0]
***** BB01
STMT00001 ( ??? ... ??? )
[000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this
[000007] n---G------ this ├──▌ IND ref
[000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0]
[000006] ----------- arg1 └──▌ LCL_VAR int V02 tmp1Fix #73615 Some regressions/improvements are expected due to more spilling in these cases.
|
|
Checked offline with @AlekseyTs and this needs to wait for a corresponding VB.NET compiler fix (C# compiler was already fixed). Will close this for now. |
Don't we need to support IL generated by older (unfixed) compilers? |
It's not that we do not support the IL, but currently we have a JIT bug that nondeterministically (depending on numbers of args, debug vs release) can sometimes evaluate things in the wrong order, and it just so happens that there is a VB.NET unit test that relies on one of these cases. |
ghost
locked as resolved and limited conversation to collaborators
|
|
||
| if (hasThis && (constraintCallThisTransform == CORINFO_DEREF_THIS)) | ||
| { | ||
| impSpillSideEffects(false, CHECK_SPILL_ALL DEBUGARG( |
There was a problem hiding this comment.
I don't recall exactly what a "global effect" is in this context. Could the same issue happen if the this pointer was from a loaded field rather than a local?
There was a problem hiding this comment.
spillGlobEffects here means to also spill nodes that read heap memory, in addition to all other side effects. We don't need to spill those because inserting an indirection doesn't modify global state.
The same issue can happen if the pointer is from a loaded field, but the spill here is still sufficient (anything that can modify global memory will be spilled even without spillGlobEffects).
| if (Result == 100) | ||
| { | ||
| Console.WriteLine("PASS"); | ||
| } | ||
| else | ||
| { | ||
| Console.WriteLine("FAIL: Got result {0}", Result); | ||
| } |
There was a problem hiding this comment.
I think the autogenerated harness will already display failure information.
Constrained calls in IL include an implicit dereference that occurs as part of the call. The JIT was adding this dereference on top of the 'this' argument tree, which makes it happen too early (before other arguments are evaluated). This changes the importer to spill when necessary to preserve ordering.
For:
Before:
After:
Fix #73615
Some regressions/improvements are expected due to more spilling in these cases.