Repository navigation
Remove tailcall limitations on unix64 and arm64 - #26255
Conversation
jakobbotsch
changed the title
|
@dotnet/jit-contrib |
|
Copying a comment from #25932: The only diffs I see on Windows x64/x86 are situations where we now insert the temps a bit earlier: N021 ( 22, 17) [000034] DA-XG-----L- * STORE_LCL_VAR ref V06 tmp1 d:1
+ [000053] ------------ START_NONGC void
+N001 ( 3, 2) [000054] ------------ t54 = LCL_VAR int V04 arg4
+ /--* t54 int
+N002 ( 4, 3) [000056] DA---------- * STORE_LCL_VAR int V07 rat0
N024 ( 1, 1) [000014] ------------ t14 = CNS_INT int 0 $40
-N001 ( 3, 2) [000053] ------------ t53 = LCL_VAR int V04 arg4
- /--* t53 int
-N002 ( 4, 3) [000055] DA---------- * STORE_LCL_VAR int V07 rat0
- [000056] ------------ START_NONGC void
/--* t14 int
[000047] ------------ * PUTARG_STK [+0x20] void
N029 ( 1, 1) [000035] ------------ t35 = LCL_VAR ref V06 tmp1 u:1 (last use) $143
/--* t35 ref
[000048] ------------ t48 = * PUTARG_REG ref REG rdx
N030 ( 1, 1) [000000] ------------ t0 = LCL_VAR ref V00 this u:1 (last use) $80
/--* t0 ref
[000049] ------------ t49 = * PUTARG_REG ref REG rcx
N031 ( 1, 1) [000012] ------------ t12 = LCL_VAR ref V03 arg3 u:1 (last use) $82
/--* t12 ref
[000050] ------------ t50 = * PUTARG_REG ref REG r8
N032 ( 3, 2) [000013] ------------ t13 = LCL_VAR int V07 rat0 $100
/--* t13 int
[000051] ------------ t51 = * PUTARG_REG int REG r9
N001 ( 3, 10) [000052] ------------ t52 = CNS_INT(h) long 0x7ffbdcbc07a0 ftn
N001 ( 3, 10) [000052] ------------ t52 = CNS_INT(h) long 0x7ffbdcbb07a0 ftn
/--* t48 ref arg1 in rdx
+--* t49 ref this in rcx
+--* t50 ref arg2 in r8
+--* t51 int arg3 in r9
+--* t52 long control expr
N037 ( 49, 33) [000015] --CXG------- * CALL void System.IO.StreamWriter..ctor $VN.VoidThis leads to swapped order in some cases: mov rdx, rax
- xor ecx, ecx
- mov r9d, ebx
G_M5466_IG03:
+ mov r9d, ebx
+ xor ecx, ecx
mov dword ptr [rsp+60H], ecx
mov rcx, rdi
mov r8, rsi
G_M5466_IG04:
add rsp, 32
pop rbx
pop rsi
pop rdi
jmp System.IO.StreamWriter:.ctor(ref,ref,int,bool):thisHowever the temps are just being created before the operand of the first |
jashook
left a comment
jashook
left a comment
There was a problem hiding this comment.
@jakobbotsch lgtm it would be nice to get throughput numbers on Windows. That way we can confirm this change does not adversely effect throughput while giving the same correctness decisions.
Thank you for the work!
|
It seems there are some issues with machines without AVX enabled -- investigating... |
|
The issue is simply #26022. I disabled |
CarolEidt
left a comment
CarolEidt
left a comment
There was a problem hiding this comment.
Overall, this LGTM. I have a couple of requests for clarifying comments.
I wonder how expensive it is to keep searching through the nodes between the each PUTARG_STK and the call. One might imagine doing something like caching the total range of offsets referenced between the first PUTARG_STK and the call, and then using that to skip an arg that has no references. (It might be more efficient to start with the last argument, and then search only the nodes between the current PUTARG_STK and the previous one, for which we've retained the info. That said, the cost may be small enough not to merit further complexity.
f37a0f7 to
082c96f
Compare
|
Thank you for the feedback @CarolEidt! I have clarified and also fixed the commit message which was using the "disjoint" terminology.
I hope the cost is small enough, I will try to collect some perf data with the help of @jashook. I think it will be somewhat complex to optimize this code in the general case without some kind of interval tree, although your idea about some caching could perhaps help in a lot of cases. However I think the expensive part is probably scanning over the LIR. The original code did this scanning, but I wonder if the JIT doesn't have up-to-date information about uses of locals at this point? I would expect it to be easier to replace uses of a local than this. |
082c96f to
3995033
Compare
|
I rebased on top of master to get the fix for #26022. Let's see if the tests pass now... |
|
@jashook helped me collect some perf data for Windows. On average it looks like it is within noise, but in the worst case (last number) it seems like this is a significant regression: - Lowering nodeinfo 1.00 30291.02 14373.603 2.65% 447.200
+ Lowering nodeinfo 1.00 30067.69 14252.139 2.58% 625.059
+ Lowering nodeinfo 1.00 33966.28 16095.635 2.46% 558.785(- is master, + is two separate runs of this PR). I imagine this case has fast tailcalls with extremely many args, though I do not know how to check which function hits the max (anyone?). |
|
This perf data is the time reported in the time log file, right? On what scenario? |
|
Yes, this is from |
|
I ran another run with |
|
PIN with "crossgen corelib" shows a 0.02% regression: https://1drv.ms/x/s!Av4baJYSo5pjgs48hU9S2fHrICK-PQ?e=Nc8Jm7 |
|
Awesome, thank you for collecting that! |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run coreclr-outerloop |
|
Commenter does not have sufficient privileges for PR 26255 in repo dotnet/coreclr |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2cbe7b2 to
b6a1f04
Compare
Fast tailcalls have their arguments passed in the incoming argument area
of the caller. This can cause problems when a previous argument might
end up overwriting the stack slot for an incoming argument that is later
used. To resolve this problem, we have logic that detects and introduces
temps in this situation. This logic was originally written for Windows
x64 where it is simple to know what argument is being overwritten, since
every argument always takes up a single slot on the stack. I.e. we know
that outgoing argument 7 can only overwrite incoming argument 7.
On unix x64 and arm64 this assumption does not hold. We previously tried
to workaround this by limiting our fast tailcalls to simple situations
where this assumption held, but this caused many missed fast tailcall
opportunities (for example, when arguments requires two slots or more).
This change removes those limitations. Instead of finding the argument
overwritten using the argument index, it keeps track of which stack
slots are used by each incoming and outgoing argument, allowing us to
more robustly check if an outgoing argument will overwrite an incoming
argument that will be used later.
To do this, we need to set the stack offset during init of args so that
we can use this info to determine whether it is necessary to introduce
temps for fast tailcalls. For arm64 we now define
FEATURE_PUT_STRUCT_ARG_STK to have access to the number of slots in
PUTARG_STK needed for this transformation.
There are also some corner cases we must consider. Since arguments now
consume multiple stack slots we can no longer move them with a single
atomic move instruction. Thus it is possible for us to get into cases
where we need to move an argument that is larger than 8 bytes and where
the move overlaps. This is a problem because codegen cannot handle
partially overlapping struct copies. We see this on unix64 in the
following case (assuming all args are on the stack):
void callee(S16 a, S32 b) { ... }
void caller(S32 a) { callee(default, a); }
Here 'caller' will need to move 'a' 16 bytes ahead in the arg list, and
we thus need a temp because we cannot do this atomically. Fix this by
detecting the partially overlapping case and looking for uses of the arg
from the current PUTARG_STK node's operand (instead of only starting
after).
b6a1f04 to
1fa41bd
Compare
|
@jashook I have fixed conflicts, would you mind taking another look? |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This change is pessimistic, but it will guarentee correctness. We specifically know that LowerFastTailCall fundamentally is broken. This has been fixed in dotnet#26255. This change exists only to make sure that we do not have SBC in older releases. Note that this changes behavoir, there are valid fast tail calls which could have avoided StackOverFlow exceptions. These tail calls must have not had overlapping slot locations, for a flawed definition of slot location. Which simply means they got lucky when generating code.
This change is pessimistic, but it will guarentee correctness. We specifically know that LowerFastTailCall fundamentally is broken. This has been fixed in dotnet#26255. This change exists only to make sure that we do not have SBC in older releases. Note that this changes behavoir, there are valid fast tail calls which could have avoided StackOverFlow exceptions. These tail calls must have not had overlapping slot locations, for a flawed definition of slot location. Which simply means they got lucky when generating code.
This change is pessimistic, but it will guarentee correctness. We specifically know that LowerFastTailCall fundamentally is broken. This has been fixed in dotnet#26255. This change exists only to make sure that we do not have SBC in older releases. Note that this changes behavoir, there are valid fast tail calls which could have avoided StackOverFlow exceptions. These tail calls must have not had overlapping slot locations, for a flawed definition of slot location. Which simply means they got lucky when generating code.
Fast tailcalls have their arguments passed in the incoming argument area
of the caller. This can cause problems when a previous argument might
end up overwriting the stack slot for an incoming argument that is later
used. To resolve this problem, we have logic that detects and introduces
temps in this situation. This logic was originally written for Windows
x64 where it is simple to know what argument is being overwritten, since
every argument always takes up a single slot on the stack. I.e. we know
that outgoing argument 7 can only overwrite incoming argument 7.
On unix x64 and arm64 this assumption does not hold. We previously tried
to workaround this by limiting our fast tailcalls to simple situations
where this assumption held, but this caused many missed fast tailcall
opportunities (for example, when arguments requires two slots or more).
This change removes those limitations. Instead of finding the argument
overwritten using the argument index, it keeps track of which stack
slots are used by each incoming and outgoing argument, allowing us to
more robustly check if an outgoing argument will overwrite an incoming
argument that will be used later.
To do this, we need to set the stack offset during init of args so that
we can use this info to determine whether it is necessary to introduce
temps for fast tailcalls. For arm64 we now define
FEATURE_PUT_STRUCT_ARG_STK to have access to the number of slots in
PUTARG_STK needed for this transformation.
There are also some corner cases we must consider. Since arguments now
consume multiple stack slots we can no longer move them with a single
atomic move instruction. Thus it is possible for us to get into cases
where we need to move an argument that is larger than 8 bytes and where
the move overlaps. This is a problem because codegen cannot handle
partially overlapping struct copies. We see this on unix64 in the
following case (assuming all args are on the stack):
void callee(S16 a, S32 b) { ... }
void caller(S32 a) { callee(default, a); }
Here 'caller' will need to move 'a' 16 bytes ahead in the arg list, and
we thus need a temp because we cannot do this atomically. Fix this by
detecting the partially overlapping case and looking for uses of the arg
from the current PUTARG_STK node's operand (instead of only starting
after).
Fixes #12468
Fixes #20726
Fixes #12644
Fixes #11717