-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix compFastTailCalls check. #35596
Fix compFastTailCalls check. #35596
Conversation
@dotnet/jit-contrib @AndyAyersMS @jakobbotsch PTAL |
With this change all but one Pri 0 x64 checked tests pass with
That test turns on on-stack replacement. I'll open an issue for that failure once this change is in. |
} | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move it even farther below and use the reportFastTailCallDecision
lambda? Then it will also go through the fast tail call decisions reporting there is in debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from @jakobbotsch 's feedback, looks good to me.
A few things for potential follow-up:
I think this method probably needs to be refactored -- the big chunk of code I added for checking implicit byrefs should be made into a helper, and we should do that check last, since it is potentially a bit more costly. But no need to do that now.
I wonder if we should modify how we handle implicit tail calls when this config setting is active, and have them be helper assisted as well. We probably get the same effect with this config plus tail call stress -- do you know if we've tried this?
Do we need to revisit the check in fgMakeOutgoingStructArgCopy
that requires that the jit makes copies for helper tail calls? I forget where we ended up on this one.
compFastTailCalls is true if COMPlus_FastTailCalls is not 0. It was introduced in dotnet#341 to help with testing: setting COMPlus_FastTailCalls to 0 forces all tail-prefixed calls to be dispatched via helpers. This fixes a subtle bug with checking compFastTailCalls: it needs to be checked after `fgInitargInfo` has been called in `fgCanFastTailCall`. `fgInitArgInfo` adds the stub address for VSD calls and `fgMorphTailCallViaHelpers` then removes it. This change also factors out the logic for checking whether the call has byref parameters that must be copied by the caller.
2b1c7b9
to
9960d15
Compare
I went ahead and did it now.
Yes, tail call stress with COMPlus_FastTailCalls set to 0 should have the same effect. I haven't tried it yet, it's on my list.
Yes, I think we can relax that check now. I'll address that separately. |
I will make sure there are no diffs with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- thanks for the refactoring.
I ran Pri0 x64 checked tests with
There are no new failures in this configuration. |
compFastTailCalls
is true ifCOMPlus_FastTailCalls
is not 0.It was introduced in #341 to help with testing:
setting
COMPlus_FastTailCalls
to 0 forces all tail-prefixedcalls to be dispatched via helpers.
This fixes a subtle bug with checking compFastTailCalls:
it needs to be checked after
fgInitargInfo
has been called infgCanFastTailCall
.fgInitArgInfo
adds the stub address for VSD callsand
fgMorphTailCallViaHelpers
then removes it.