Uh oh!
There was an error while loading. Please reload this page.
JIT: fix overly aggressive tail recursive call loop marking - #33517
Conversation
If there is a tail recursive call site, the jit will mark all blocks from method entry to the block with the call as "in a loop," anticipating a tail recursive call to loop optimization. Because of some confusing naming we were doing this even for recursive calls that were not tail calls. Upshot is that blocks were marked as being in loops when they weren't, and (among other things) this made the inliner more aggressive for calls in those blocks.
@dotnet/jit-contrib PTAL. This fixes an issue I ran into while working on on stack replacement, see these review comments. I'll mark the one semantic change with a review comment; the remainder is renaming and compressing some dumps via JITDUMP. Jit-diffs shows reduction in code size. At first glance most of this is because the jit is doing fewer inlines as fewer blocks in recursive methods are considered to be "in a loop." |
| // A tail recursive call is a potential loop from the current block to the start of the method. | ||
| if (canTailCall && gtIsRecursiveCall(methHnd)) | ||
| if ((tailCallFlags != 0) && canTailCall && gtIsRecursiveCall(methHnd)) |
There was a problem hiding this comment.
This is the actual fix -- canTailCall is set to true at the top of a method, and so might still be true when we reach this point for calls that are not tail calls. So we need to also check if this call is a tail call.
Uh oh!
There was an error while loading. Please reload this page.
CarolEidt
left a comment
There was a problem hiding this comment.
LGTM. I gather you've sampled to diffs to ensure that they are as expected?
AndyAyersMS
commented
Mar 12, 2020
Wasn't sure what to expect. Checked what I hope is a representative sample; all of the diffs I saw were from less inlining, and quite often, from less inlining on exception paths. Here's an example from System.Private.Xml: mov rcx, 0xD1FFAB1E
call CORINFO_HELP_NEWSFAST
mov rdi, rax
- mov rcx, rsi- xor rdx, rdx- call XPathException:CreateMessage(String,ref):String- mov dword ptr [rdi+112], 0xD1FFAB1E- mov dword ptr [rdi+116], 0xD1FFAB1E- lea rcx, bword ptr [rdi+16]- mov rdx, rax- call CORINFO_HELP_ASSIGN_REF- xor rdx, rdx- mov gword ptr [rdi+32], rdx- mov dword ptr [rdi+116], 0xD1FFAB1E- mov dword ptr [rdi+116], 0xD1FFAB1E- lea rcx, bword ptr [rdi+120]+ mov rcx, rdi
mov rdx, rsi
- call CORINFO_HELP_ASSIGN_REF- xor rcx, rcx- mov gword ptr [rdi+128], rcx+ xor r8, r8+ xor r9, r9+ call XPathException:.ctor(String,ref,Exception):this
mov rcx, rdi
call CORINFO_HELP_THROW
int3
-; Total bytes of code 779, prolog size 10, PerfScore 155.04, (MethodHash=14ab57cd) for method QueryBuilder:ProcessNode(AstNode,int,byref):Query:this+; Total bytes of code 632, prolog size 10, PerfScore 133.46, (MethodHash=14ab57cd) for method QueryBuilder:ProcessNode(AstNode,int,byref):Query:thisIn some of the "regression" cases, eg |
erozenfeld
commented
Mar 12, 2020
I would expect some diffs to show eliminated zero-inits since we rely on BBF_BACKWARD_JUMP when deciding whether we can eliminate explicit zero initializations. |
erozenfeld
commented
Mar 12, 2020
We can make this check even less aggressive if we take into account other conditions that currently block tail recursion --> loop optimization: runtime/src/coreclr/src/jit/morph.cpp Lines 7308 to 7312 in 375d137 |
| printf("\n"); | ||
| } | ||
| #endif | ||
| JITDUMP("\nGTF_CALL_M_EXPLICIT_TAILCALL set for call [%06u]\n", dspTreeID(call)); |
There was a problem hiding this comment.
dspTreeID() really should be named getTreeID()
briansull
left a comment
There was a problem hiding this comment.
LGTM - Thanks for finding and fixing this
Was looking for diffs like this, but haven't spotted any. Will sample a few more. I've also been thinking we need a jit-diff metric for prolog zeroing, which might make it easier to see this sort of thing. We have one for prolog size but it's not quite the same, and I think is broken.
Sure, perhaps in a follow-on PR? I'm also a bit worried that we are setting |
erozenfeld
commented
Mar 12, 2020
In this case the zero-init diffs will be in the main body, not in the prolog. This change should only affect elimination of explicit zero-inits.
Yes, I think we might have this problem at the place where we call runtime/src/coreclr/src/jit/morph.cpp Lines 8128 to 8170 in 375d137
Sounds good. |
Also incorporate fix from dotnet#33517.
If there is a tail recursive call site, the jit will mark all blocks
from method entry to the block with the call as "in a loop," anticipating
a tail recursive call to loop optimization.
Because of some confusing naming we were doing this even for recursive calls
that were not tail calls. Upshot is that blocks were marked as being in loops
when they weren't, and (among other things) this made the inliner more
aggressive for calls in those blocks.