Skip to content
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

[RyuJIT] TailcallStress needs to check more conditions #8017

Closed
BruceForstall opened this issue May 4, 2017 · 6 comments · Fixed by #40698
Closed

[RyuJIT] TailcallStress needs to check more conditions #8017

BruceForstall opened this issue May 4, 2017 · 6 comments · Fixed by #40698
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug JitStress CLR JIT issues involving JIT internal stress modes
Milestone

Comments

@BruceForstall
Copy link
Member

When COMPlus_TailcallStress=1 is set, the JIT converts as many call/ret pairs as possible into tail call, which it treats as tail. prefixed "explicit" tail calls. It does not in this case run the legality checks that are run when checking to determine if implicit tailcall optimization can be done, specifically the code under FEATURE_TAILCALL_OPT in fgMorphCall() that rejects a potential tail call if any variable in the function has had its address taken, if there are pinned locals, or if there are struct promoted struct arguments.

This causes code such as this to fail:

    unsafe class Program
    {
        static void foo(IntPtr x)
        {
            Console.WriteLine(*(int*)x);
        }
        static void Main(string[] args)
        {
            int x = 42;
            foo((IntPtr)(&x));
        }
    }

category:correctness
theme:tail-call
skill-level:intermediate
cost:small

@leppie
Copy link
Contributor

leppie commented May 4, 2017

A tail call will implicitly discard the stack, so I am not sure how this can be a viable candidate.

PS: Also this is just a tail call, not a tail call optimization. This will in fact run slower than a normal direct call in the current JIT. TCO is an ambiguous term that should not be used.

@BruceForstall
Copy link
Member Author

@leppie yes, the JIT bug is that in this particular stress mode, the JIT does not do enough legality checking, therefore makes an illegal tail call.

@BruceForstall
Copy link
Member Author

(Internal note: this came from VSO#278840)

@BruceForstall
Copy link
Member Author

If/when fixed, enable the test added with dotnet/coreclr#11410

pgavlin referenced this issue in pgavlin/coreclr May 16, 2017
Tail call stress does not mix well with ZapDisable due to the issues
described in #11408.

Fixes #11648.
pgavlin referenced this issue in dotnet/coreclr May 16, 2017
* Disable tail call stress in GH_10780 if ZapDisable is enabled.

Tail call stress does not mix well with ZapDisable due to the issues
described in #11408.

Fixes #11648.

* Add a missing semicolon.
JosephTremoulet referenced this issue in JosephTremoulet/coreclr May 24, 2017
Update fgMorphImplicitByRefArgs to rewrite references to struct-promoted
implicit-by-reference parameters as references to the new struct temps
created in fgRetypeImplicitByRefArgs; fgMorphStructField isn't updating
these because it's only interested in field references, and runs upstream
of fgRetypeImplicitByRefArgs where the full struct temp is created,
anyway.
Invert the sense of lvPromoted for implicit byref args in the interim
between fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs,
since now fgMarkDemotedImplicitByRefArgs needs to update both and would
look horribly backwards the other way around.

Fixes #11408.
pgavlin referenced this issue in pgavlin/coreclr Jun 5, 2017
Tail call stress does not mix well with ZapDisable due to the issues
described in #11408.
pgavlin referenced this issue in pgavlin/coreclr Jun 5, 2017
…if ZapDisable is enabled.

Tail call stress does not mix well with ZapDisable due to the issues
described in #11408.
@echesakov echesakov self-assigned this Jun 12, 2018
BruceForstall referenced this issue in BruceForstall/coreclr Jul 26, 2018
Fixes #13796

Note that test GitHub_11408 is still disabled, now against issue #11408.
BruceForstall referenced this issue in BruceForstall/coreclr Jul 26, 2018
Fixes #13796, #12549

Note that test GitHub_11408 is still disabled, now against issue #11408.
jashook referenced this issue in jashook/coreclr Aug 14, 2018
Fixes #13796, #12549

Note that test GitHub_11408 is still disabled, now against issue #11408.
@BruceForstall
Copy link
Member Author

@jakobbotsch @jashook fyi

@jakobbotsch
Copy link
Member

IMO it does not make sense to spend time on trying to make tailcall stress work -- instead ABI stress should be expanded to cover the missing cases (generating tests that are correct by construction).

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@echesakov echesakov removed their assignment Aug 14, 2020
erozenfeld added a commit to erozenfeld/runtime that referenced this issue Aug 14, 2020
Improve validation of tail calls that are not tail-prefixed in the IL
but are marked as such because of TailCallStress. We now do the same
correctness validation in morph for such tail calls as we do for
implicit tail calls. That blocks tail calls when we have address-taken
locals, struct promoted params, and pinned vars.

Fixes dotnet#39398.
Fixes dotnet#39309.
Fixes dotnet#38892.
Fixes dotnet#38889.
Fixes dotnet#38887.
Fixes dotnet#37117.
Fixes dotnet#8017.
erozenfeld added a commit that referenced this issue Aug 15, 2020
Improve validation of tail calls that are not tail-prefixed in the IL
but are marked as such because of TailCallStress. We now do the same
correctness validation in morph for such tail calls as we do for
implicit tail calls. That blocks tail calls when we have address-taken
locals, struct promoted params, and pinned vars.

Fixes #39398.
Fixes #39309.
Fixes #38892.
Fixes #38889.
Fixes #38887.
Fixes #37117.
Fixes #8017.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
ViktorHofer pushed a commit to dotnet/winforms that referenced this issue Dec 5, 2022
Improve validation of tail calls that are not tail-prefixed in the IL
but are marked as such because of TailCallStress. We now do the same
correctness validation in morph for such tail calls as we do for
implicit tail calls. That blocks tail calls when we have address-taken
locals, struct promoted params, and pinned vars.

Fixes dotnet/runtime#39398.
Fixes dotnet/runtime#39309.
Fixes dotnet/runtime#38892.
Fixes dotnet/runtime#38889.
Fixes dotnet/runtime#38887.
Fixes dotnet/runtime#37117.
Fixes dotnet/runtime#8017.

Commit migrated from dotnet/runtime@7742b57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug JitStress CLR JIT issues involving JIT internal stress modes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants