[RISC-V] New ABI classifiers #101114

tomeksowi · 2024-04-16T11:39:24Z

Implements the RISC-V part of #100744

Part of #84834, cc @dotnet/samsung

dotnet-policy-service · 2024-04-16T11:39:50Z

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

src/coreclr/jit/abi.cpp

jakobbotsch · 2024-04-16T11:56:43Z

Thanks for this, very happy to see it.
This change should allow you to remove the RISCV version of genHomeRegisterParams and switch to the platform agnostic version in codegencommon.cpp.

…ist doesn't guarantee no heap allocation in C++11 (only since C++14)

shushanhf · 2024-04-17T03:12:37Z

Thanks for this, very happy to see it. This change should allow you to remove the RISCV version of genHomeRegisterParams and switch to the platform agnostic version in codegencommon.cpp.

I agree with you that we can share the genHomeRegisterParams for different arches.
But we can not optimize by load/store-index instruction at some cases if we use architecture-independent code.

jakobbotsch · 2024-04-17T07:20:00Z

But we can not optimize by load/store-index instruction at some cases if we use architecture-independent code.

What is this optimization?

shushanhf · 2024-04-17T07:43:33Z

But we can not optimize by load/store-index instruction at some cases if we use architecture-independent code.

What is this optimization?

As we use emitIns_S_R() to store one by one, if the offset is large offset which out of the store ins's imm encoding range, we have to generate an ins to encode the large offset for each store while we can use the load-index to avoid redundant large offset's ins.
This is the root case making the Prolog size very large and the LoongArch64 had optimized these.

jakobbotsch · 2024-04-17T08:18:28Z

As we use emitIns_S_R() to store one by one, if the offset is large offset which out of the store ins's imm encoding range, we have to generate an ins to encode the large offset for each store while we can use the load-index to avoid redundant large offset's ins. This is the root case making the Prolog size very large and the LoongArch64 had optimized these.

What does the codegen look like? Does it set up a temporary register pointing closer to the parameters than FP and store based on this register instead?

Is it still relevant after Adjust the calleeSavedRegs on top frame for LoongArch64/RISCV64 #100962?
If it is, can we introduce emitIns_StoreParamToStack and LA64/RISCV64 can preface the logic in codegencommon.cpp by setting up the temporary register, and then do its optimization to use that register within emitIns_StoreParamToStack?

Keeping separate version of genHomeRegisterParams means high likelihood of us breaking LA64/RISCV64 in the future. The new version was written partially to support parameters in non-standard registers. PRs like #100823 will break LA64/RISCV64 as long as they have their own version of this function.

shushanhf · 2024-04-17T08:27:33Z

As we use emitIns_S_R() to store one by one, if the offset is large offset which out of the store ins's imm encoding range, we have to generate an ins to encode the large offset for each store while we can use the load-index to avoid redundant large offset's ins. This is the root case making the Prolog size very large and the LoongArch64 had optimized these.

What does the codegen look like? Does it set up a temporary register pointing closer to the parameters than FP and store based on this register instead?

yes, you are right.

Is it still relevant after Adjust the calleeSavedRegs on top frame for LoongArch64/RISCV64 #100962?

Thanks.
I checked and it is not relevant with 100962. As the special offs of emitIns_S_R() is indepent of the offset relative FP.

If it is, can we introduce emitIns_StoreParamToStack and LA64/RISCV64 can preface the logic in codegencommon.cpp by setting up the temporary register, and then do its optimization to use that register within emitIns_StoreParamToStack?

Yes, that's a good idea that introducing a new interface to do that.

Keeping separate version of genHomeRegisterParams means high likelihood of us breaking LA64/RISCV64 in the future.

I agree with you for sharing the genHomeRegisterParams.

src/coreclr/jit/abi.h

jakobbotsch · 2024-04-17T09:16:08Z

I checked and it is not relevant with 100962.

Are you saying that #100962 doesn't help? I would expect it to make the optimization unimportant because it changes the FP to sit right next to where the parameter locals are allocated on the stack, so the relative offset from the FP is very small now.

As the special offs of emitIns_S_R() is indepent of the offset relative FP.

The offset provided to emitIns_S_R by register homing should be very small, 0 to 8 for structs passed in registers on RISCV64/LA64 if I understand the ABI correctly.

jakobbotsch · 2024-04-17T09:18:10Z

src/coreclr/jit/targetriscv64.cpp

+
+    assert((floatFields > 0) || (intFields == 0));
+
+    auto PassSlot = [this](bool inFloatReg, unsigned offset, unsigned size) -> ABIPassingSegment {


Nit: Most places within the JIT that use lambdas use camelCase naming convention.

No need to address this here.

jakobbotsch · 2024-04-17T09:19:10Z

src/coreclr/jit/targetriscv64.cpp

+        }
+    }
+
+    unreached();


Does this do anything? Won't we have a compiler error if this becomes accidentally reachable?

Feel free to remove this separately.

jakobbotsch

Thank you!

shushanhf · 2024-04-17T09:30:40Z

Are you saying that #100962 doesn't help? I would expect it to make the optimization unimportant because it changes the FP to sit right next to where the parameter locals are allocated on the stack, so the relative offset from the FP is very small now.

Yes, from this view, after 100962 the args' position is more near the FP.

* Code review from #101114 * Use common genHomeRegisterParams on RISC-V * Make passSlot integer-only because we know hardware floating-point calling convention passes in registers only * Make a RISC-V specific routine for homing stack parts of split parameters. * Move genHomeStackPartOfSplitParameter out of genHomeSwiftStructParameters, share stack segment homing with Swift code

* Code review from dotnet#101114 * Use common genHomeRegisterParams on RISC-V * Make passSlot integer-only because we know hardware floating-point calling convention passes in registers only * Make a RISC-V specific routine for homing stack parts of split parameters. * Move genHomeStackPartOfSplitParameter out of genHomeSwiftStructParameters, share stack segment homing with Swift code

tomeksowi added 4 commits April 16, 2024 11:46

Add new style ABI classifiers

7a90c59

Format

8c76b09

Add ABIPassingInformation::FromSegment

f078e9b

Factor out float reg segment passing

68049e9

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 16, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 16, 2024

jakobbotsch reviewed Apr 16, 2024

View reviewed changes

src/coreclr/jit/abi.cpp Outdated Show resolved Hide resolved

clamp03 assigned tomeksowi Apr 16, 2024

clamp03 added the arch-riscv Related to the RISC-V architecture label Apr 16, 2024

Remove ABIPassingInformation::FromSegments because std::initializer_l…

9ce8cab

…ist doesn't guarantee no heap allocation in C++11 (only since C++14)

This was referenced Apr 16, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

System.Text.RegularExpressions.Tests on Mono_Minijit_Debug-Ubuntu #100715

Closed

tomeksowi marked this pull request as ready for review April 17, 2024 06:13

Bajtazar reviewed Apr 17, 2024

View reviewed changes

src/coreclr/jit/abi.h Show resolved Hide resolved

jakobbotsch reviewed Apr 17, 2024

View reviewed changes

jakobbotsch approved these changes Apr 17, 2024

View reviewed changes

jakobbotsch merged commit f930b15 into dotnet:main Apr 17, 2024
108 of 110 checks passed

This was referenced Apr 17, 2024

Simplify "secret stub arg" handling between JIT and EE #100823

Merged

JIT: Add missing new-style ABI classifiers for LA64 and RISCV64 #100744

Closed

tomeksowi added a commit to tomeksowi/runtime that referenced this pull request Apr 19, 2024

Code review from dotnet#101114

25ab569

tomeksowi mentioned this pull request Apr 19, 2024

[RISC-V] Use common CodeGen::genHomeRegisterParams #101288

Merged

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024

[RISC-V] New ABI classifier (dotnet#101114)

e223770

github-actions bot locked and limited conversation to collaborators May 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] New ABI classifiers #101114

[RISC-V] New ABI classifiers #101114

tomeksowi commented Apr 16, 2024

dotnet-policy-service bot commented Apr 16, 2024

jakobbotsch commented Apr 16, 2024

shushanhf commented Apr 17, 2024 •

edited

Loading

jakobbotsch commented Apr 17, 2024

shushanhf commented Apr 17, 2024 •

edited

Loading

jakobbotsch commented Apr 17, 2024

shushanhf commented Apr 17, 2024 •

edited

Loading

jakobbotsch commented Apr 17, 2024

jakobbotsch Apr 17, 2024

jakobbotsch Apr 17, 2024

jakobbotsch left a comment

shushanhf commented Apr 17, 2024


		assert((floatFields > 0) \|\| (intFields == 0));

		auto PassSlot = [this](bool inFloatReg, unsigned offset, unsigned size) -> ABIPassingSegment {

[RISC-V] New ABI classifiers #101114

[RISC-V] New ABI classifiers #101114

Conversation

tomeksowi commented Apr 16, 2024

dotnet-policy-service bot commented Apr 16, 2024

jakobbotsch commented Apr 16, 2024

shushanhf commented Apr 17, 2024 • edited Loading

jakobbotsch commented Apr 17, 2024

shushanhf commented Apr 17, 2024 • edited Loading

jakobbotsch commented Apr 17, 2024

shushanhf commented Apr 17, 2024 • edited Loading

jakobbotsch commented Apr 17, 2024

jakobbotsch Apr 17, 2024

Choose a reason for hiding this comment

jakobbotsch Apr 17, 2024

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

shushanhf commented Apr 17, 2024

shushanhf commented Apr 17, 2024 •

edited

Loading

shushanhf commented Apr 17, 2024 •

edited

Loading

shushanhf commented Apr 17, 2024 •

edited

Loading