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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from
+121 −36

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.

@risc-vv
Copy link

risc-vv commented May 23, 2025 •
edited
Loading

RISC-V Release-CLR-QEMU: 9077 / 9107 (99.67%) 
=======================
      passed: 9077
      failed: 2
     skipped: 597
      killed: 28
------------------------
 TOTAL tests: 9704
VIRTUAL time: 36h 2min 56s 934ms
   REAL time: 36min 51s 117ms
=======================

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

Build information and commands

GIT: 84866ac56d9d8448b0c376119ec5a7163c8c99fe
CI: 85a71e207aad1e1aa4a72cc7f52d713d9cc79191
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

RISC-V Release-CLR-VF2: 9078 / 9108 (99.67%) 
=======================
      passed: 9078
      failed: 2
     skipped: 597
      killed: 28
------------------------
 TOTAL tests: 9705
VIRTUAL time: 10h 41min 44s 144ms
   REAL time: 43min 36s 351ms
=======================

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

Build information and commands

GIT: 84866ac56d9d8448b0c376119ec5a7163c8c99fe
CI: 85a71e207aad1e1aa4a72cc7f52d713d9cc79191
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

RISC-V Release-FX-QEMU: 269204 / 270233 (99.62%) 
=======================
      passed: 269204
      failed: 1021
     skipped: 38
      killed: 8
------------------------
 TOTAL tests: 270271
VIRTUAL time: 29h 20min 41s 530ms
   REAL time: 1h 11min 25s 688ms
=======================

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

Build information and commands

GIT: 84866ac56d9d8448b0c376119ec5a7163c8c99fe
CI: 85a71e207aad1e1aa4a72cc7f52d713d9cc79191
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

RISC-V Release-FX-VF2: 299077 / 300793 (99.43%) 
=======================
      passed: 299077
      failed: 1708
     skipped: 38
      killed: 8
------------------------
 TOTAL tests: 300831
VIRTUAL time: 18h 13min 53s 102ms
   REAL time: 2h 12min 20s 831ms
=======================

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

Build information and commands

GIT: 84866ac56d9d8448b0c376119ec5a7163c8c99fe
CI: 85a71e207aad1e1aa4a72cc7f52d713d9cc79191
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@jakobbotsch jakobbotsch Awaiting requested review from jakobbotsch

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
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.
3 participants
@tomeksowi @risc-vv @am11