Skip to content

[RISC-V][LoongArch64] Handle reversed fields in lowered structs #115933

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
Merged
merged 2 commits into from
May 28, 2025
Merged

[RISC-V][LoongArch64] Handle reversed fields in lowered structs #115933

merged 2 commits into from
May 28, 2025

Conversation

tomeksowi
Copy link
Contributor

  1. Lower structs with reversed fields for passing according to the hardware floating-point calling convention
  2. Fix GC info when GC pointer is passed int the second field of an int-float struct like JIT: Fix SysV first/second return register GC info mismatch when generating calls #115827 for SysV

Part of #84834, cc @dotnet/samsung @LuckyXu-HF @shushanhf

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 23, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 23, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@am11 am11 added arch-loongarch64 arch-riscv Related to the RISC-V architecture labels May 23, 2025
tomeksowi
tomeksowi commented May 23, 2025
View reviewed changes
src/coreclr/vm/methodtable.cpp
Comment on lines +2951 to +2962
if (nFields == 2 && info.offset1st > info.offset2nd)
{
LOG((LF_JIT, LL_EVERYTHING, "FpStructInRegistersInfo: struct %s (%u bytes): swap fields to match memory order\n",
(!th.IsTypeDesc() ? th.AsMethodTable() : th.AsNativeValueType())->GetDebugClassName(), th.GetSize()));
info.flags = FpStruct::Flags(
((info.flags & FloatInt) << (PosIntFloat - PosFloatInt)) |
((info.flags & IntFloat) >> (PosIntFloat - PosFloatInt)) |
((info.flags & SizeShift1stMask) << (PosSizeShift2nd - PosSizeShift1st)) |
((info.flags & SizeShift2ndMask) >> (PosSizeShift2nd - PosSizeShift1st))
);
std::swap(info.offset1st, info.offset2nd);
}
Copy link
Contributor Author

@tomeksowi tomeksowi May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order matters for two floats and the calling convention wasn't clear. The only precedent I could find was GCC/gnat compiling Ada of all places, I made a proposal following it: riscv-non-isa/riscv-elf-psabi-doc#463, LoongArch gnat also behaves this way.

For int-float structs it doesn't matter but I'm ordering the fields as they come in memory anyway for consistency.

@am11 am11 requested a review from jakobbotsch May 23, 2025 10:34
@build-analysis build-analysis bot mentioned this pull request May 23, 2025
Open
3 tasks
jakobbotsch
jakobbotsch reviewed May 27, 2025
View reviewed changes
src/coreclr/tools/Common/Internal/Runtime/RiscVLoongArch64FpStruct.cs
Comment on lines +210 to +220
if (nFields == 2 && info.offset1st > info.offset2nd)
{
// swap fields to match memory order
info.flags = (FpStruct)(
((uint)(info.flags & FloatInt) << (PosIntFloat - PosFloatInt)) |
((uint)(info.flags & IntFloat) >> (PosIntFloat - PosFloatInt)) |
((uint)(info.flags & SizeShift1stMask) << (PosSizeShift2nd - PosSizeShift1st)) |
((uint)(info.flags & SizeShift2ndMask) >> (PosSizeShift2nd - PosSizeShift1st))
);
(info.offset2nd, info.offset1st) = (info.offset1st, info.offset2nd);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is nFields == 2 the only case that needs to be handled? What about e.g. 8 byte fields and a double field? Can that combination not be passed in an integer + float register?

Copy link
Contributor Author

@tomeksowi tomeksowi May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is nFields == 2 the only case that needs to be handled?

Yes

What about e.g. 8 byte fields and a double field? Can that combination not be passed in an integer + float register?

Only structs with one or two primitive fields (at least one of them FP) are lowered, structs with more fields are passed in integer registers and/or on the stack as they are laid out in memory.

https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/87aecf601722171c570120a46003be3c17ad3108/riscv-cc.adoc?plain=1#L316-L336

@risc-vv
Copy link

risc-vv commented May 27, 2025 •
edited
Loading

RISC-V Release-CLR-QEMU: 9075 / 9105 (99.67%) 
=======================
      passed: 9075
      failed: 2
     skipped: 597
      killed: 28
------------------------
 TOTAL tests: 9702
VIRTUAL time: 35h 9min 19s 925ms
   REAL time: 35min 54s 614ms
=======================

report.xml, report.md, failures.xml, testclr_details.tar.zst

RISC-V Release-CLR-VF2: 9076 / 9106 (99.67%) 
=======================
      passed: 9076
      failed: 2
     skipped: 597
      killed: 28
------------------------
 TOTAL tests: 9703
VIRTUAL time: 10h 23min 27s 702ms
   REAL time: 42min 23s 111ms
=======================

report.xml, report.md, failures.xml, testclr_details.tar.zst

RISC-V Release-FX-QEMU: 282588 / 283649 (99.63%) 
=======================
      passed: 282588
      failed: 1056
     skipped: 38
      killed: 5
------------------------
 TOTAL tests: 283687
VIRTUAL time: 28h 25min 36s 128ms
   REAL time: 1h 11min 14s 651ms
=======================

report.xml, report.md, failures.xml, testclr_details.tar.zst

RISC-V Release-FX-VF2: 308940 / 310658 (99.45%) 
=======================
      passed: 308940
      failed: 1708
     skipped: 38
      killed: 10
------------------------
 TOTAL tests: 310696
VIRTUAL time: 20h 38min 11s 143ms
   REAL time: 2h 4min 40s 918ms
=======================

report.xml, report.md, failures.xml, testclr_details.tar.zst

Build information and commands

GIT: 84866ac56d9d8448b0c376119ec5a7163c8c99fe
CI: 8d15504ae1358a01693a7c50a10ed6536bf11989
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

@jakobbotsch
Copy link
Member

/ba-g Failure is #115955

@jakobbotsch jakobbotsch merged commit 9376d46 into dotnet:main May 28, 2025
110 of 112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobbotsch jakobbotsch

@MichalStrehovsky MichalStrehovsky

Assignees

@tomeksowi tomeksowi

Labels
arch-loongarch64 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.
4 participants
@tomeksowi @risc-vv @jakobbotsch @am11