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] NaN-box float arguments #93665

Merged
merged 4 commits into from
Oct 21, 2023
Merged

[RISC-V] NaN-box float arguments #93665

merged 4 commits into from
Oct 21, 2023

Conversation

tomeksowi
Copy link
Contributor

  • NaN-box the arguments passed to CallDescrWorkerInternal because that stub is unaware of floating-point argument sizes

  • NaN-box floats in CopyStructToRegisters

  • Use variant of fmv.?.x appropriate for the size of the floating-point argument in function prolog

    Floats passed in integer registers need to be NaN-boxed.

This fixes System.Text.Json.Tests.Utf8JsonWriterTests.(WriteNumberValueSingle|WriteFloatValue)

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov @clamp03 @sirntar @yurai007
cc @shushanhf for changes in CopyStructToRegisters

@tomeksowi
[RISC-V] NaN-box the arguments passed to CallDescrWorkerInternal beca…
0290b73 
…use that stub is unaware of floating-point argument sizes
@tomeksowi
[RISC-V] Use variant of fmv.?.x appropriate for the size of the float…
de178af 
…ing-point argument

Floats passed in integer registers need to be NaN-boxed.
@tomeksowi
[RISC-V] NaN-box floats in CopyStructToRegisters
fe923b9
@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 needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 18, 2023
@build-analysis build-analysis bot mentioned this pull request Oct 18, 2023
Closed
@danmoseley danmoseley added area-Codegen-JIT-mono arch-riscv Related to the RISC-V architecture and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 18, 2023
@tomeksowi tomeksowi marked this pull request as ready for review October 19, 2023 06:35
@shushanhf
Copy link
Contributor

This fixes System.Text.Json.Tests.Utf8JsonWriterTests.(WriteNumberValueSingle|WriteFloatValue)

Part of #84834 cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov @clamp03 @sirntar @yurai007 cc @shushanhf for changes in CopyStructToRegisters

We did't meet this error.
We test the System.Text.Json.Tests on LA64 and it is OK for LA64 even not adding this PR.

@tomeksowi
Copy link
Contributor Author

We did't meet this error.
We test the System.Text.Json.Tests on LA64 and it is OK for LA64 even not adding this PR.

Good to hear. This PR should be neutral then, CopyStructToRegisters ORs with 0 on LA64, rest is RISCV-specific.

@tomeksowi
Copy link
Contributor Author

PR ready to review, @jakobbotsch JIT part, @jkotas(?) VM.

@tomeksowi tomeksowi requested a review from jkotas October 20, 2023 09:55
@jkotas
Copy link
Member

jkotas commented Oct 20, 2023

The VM change LGTM. @jakobbotsch Could you please review the JIT change?

@jkotas jkotas added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-Codegen-JIT-mono labels Oct 20, 2023
@ghost
Copy link

ghost commented Oct 20, 2023

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

Issue Details

  • NaN-box the arguments passed to CallDescrWorkerInternal because that stub is unaware of floating-point argument sizes

  • NaN-box floats in CopyStructToRegisters

  • Use variant of fmv.?.x appropriate for the size of the floating-point argument in function prolog

    Floats passed in integer registers need to be NaN-boxed.

This fixes System.Text.Json.Tests.Utf8JsonWriterTests.(WriteNumberValueSingle|WriteFloatValue)

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov @clamp03 @sirntar @yurai007
cc @shushanhf for changes in CopyStructToRegisters

Author: tomeksowi
Assignees: tomeksowi
Labels:

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

This was referenced Oct 20, 2023
Closed
Open
Closed
@jakobbotsch jakobbotsch merged commit 9a6d530 into dotnet:main Oct 21, 2023
126 of 129 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@jkotas jkotas Awaiting requested review from jkotas

Assignees

@tomeksowi tomeksowi

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.
5 participants
@tomeksowi @shushanhf @jkotas @jakobbotsch @danmoseley