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.

Sign up for GitHub

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

Jump to bottom

[RISC-V] Disable FastTailCall in case of split arg #93655

Merged
merged 1 commit into from
Oct 27, 2023
Merged

[RISC-V] Disable FastTailCall in case of split arg #93655

merged 1 commit into from
Oct 27, 2023

Conversation

t-mustafin
Copy link
Contributor

@t-mustafin t-mustafin commented Oct 18, 2023 •
edited
Loading

Disable TCO in case if callee split arg fixes System.Security.Cryptography.Tests SIGSEGV/Release and assert/Checked.
Also disable TCO in case if caller split arg as for ARM32: #66282.

Part of #84834
cc @clamp03 @gbalykov @tomeksowi @sirntar @viewizard @ashaurtaev.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 18, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Disable TCO in case if callee split arg fixes System.Security.Cryptography.Tests SIGSEGV/Release and assert/Checked.
Also disable TCO in case if caller split arg as for ARM32: #66282.

cc @clamp03 @gbalykov @tomeksowi @sirntar @viewizard @ashaurtaev.

Author: t-mustafin
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

@t-mustafin
Copy link
Contributor Author

@shushanhf Loongarch64 has the same SplitArg behaviour as Riscv64. Does Loongarch64 also needs this disable or Loongarch64 has proper TCO handling on SplitArg case?

@build-analysis build-analysis bot mentioned this pull request Oct 18, 2023
Closed
@shushanhf
Copy link
Contributor

shushanhf commented Oct 19, 2023 •
edited
Loading

@shushanhf Loongarch64 has the same SplitArg behaviour as Riscv64. Does Loongarch64 also needs this disable or Loongarch64 has proper TCO handling on SplitArg case?

Thanks!

We did't meet this error.
We test the System.Security.Cryptography.Tests on runtime8.0-rc2, but is OK for LA64 even not adding this PR.
Do you test it on the main9.0 or the runtime8.0-rc2 ?

@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Oct 19, 2023
@t-mustafin
Copy link
Contributor Author

Do you test it on the main9.0 or the runtime8.0-rc2 ?

We test on main9.0. Thanks for information.
Problem occurs with Tier-1 compilation executed for TCO-compatible method in case of callee has splitArg:

System.Security.Cryptography.SP800108HmacCounterKdf:DeriveBytesCore(System.ReadOnlySpan`1[ubyte],System.Security.Cryptography.HashAlgorithmName,System.ReadOnlySpan`1[ubyte],System.ReadOnlySpan`1[ubyte],System.Span`1[ubyte]) (MethodHash=b612d031)

@t-mustafin t-mustafin marked this pull request as ready for review October 25, 2023 11:20
@build-analysis build-analysis bot mentioned this pull request Oct 25, 2023
Open
@t-mustafin t-mustafin marked this pull request as draft October 27, 2023 01:06
@t-mustafin t-mustafin marked this pull request as ready for review October 27, 2023 01:11
This was referenced Oct 27, 2023
Closed
Open
@jakobbotsch
Copy link
Member

Can you run jit-format?

@jakobbotsch jakobbotsch merged commit 5f0fe49 into dotnet:main Oct 27, 2023
127 of 129 checks passed
@t-mustafin t-mustafin deleted the riscv_disable_TCO_with_split branch October 30, 2023 10:16
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
@t-mustafin @liveans
[RISC-V] Disable FastTailCall in case of split arg (dotnet#93655)
8aa5a49
@gbalykov
Copy link
Member

@jkotas can you please add @t-mustafin to dotnet org?

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomeksowi tomeksowi tomeksowi approved these changes

@clamp03 clamp03 clamp03 approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@gbalykov gbalykov gbalykov approved these changes

@ashaurtaev ashaurtaev ashaurtaev approved these changes

Assignees

@t-mustafin t-mustafin

Labels
arch-riscv Related to the RISC-V architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@t-mustafin @shushanhf @jakobbotsch @gbalykov @tomeksowi @clamp03 @ashaurtaev