[RISC-V] Fix errors in crosgen2 for risc-v #97368

clamp03 · 2024-01-23T08:07:08Z

Fixed some errors in crossgen2 compiling System.Private.CoreLib.dll and running helloworld with compiled SPC.dll
Crossgen2 for risc-v still has many bugs to be fixed. Actually, CoreCLR tests with crossgen2 are not passed yet.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @gbalykov @ashaurtaev @tomeksowi @sirntar @yurai007 @Bajtazar

A failure during crossgen2 SPC.dll `System.Diagnostics.Tracing.NativeRuntimeEventSource:LogThreadPoolWorkerThreadAdjustmentStats(double,double,double,double,double,double,double,double,double,ushort,ushort)`

src/coreclr/vm/riscv64/stubs.cpp

clamp03 · 2024-01-23T12:02:35Z

@shushanhf It updats codes near LOONGARCH64. Please confirm that it doesn't break LOONGARCH64. And I think this PR can help you to update crossgen2 for LOONGARCH64.

ryujit-bot · 2024-01-23T14:06:55Z

Diff results for #97368

Throughput diffs

Throughput diffs for windows/x86 ran on windows/x86

Overall (+0.00% to +0.01%)

Collection	PDIFF
benchmarks.run_tiered.windows.x86.checked.mch	+0.01%
coreclr_tests.run.windows.x86.checked.mch	+0.01%
libraries.crossgen2.windows.x86.checked.mch	+0.01%
libraries.pmi.windows.x86.checked.mch	+0.01%
libraries_tests.run.windows.x86.Release.mch	+0.01%
realworld.run.windows.x86.checked.mch	+0.01%

MinOpts (+0.01%)

Collection	PDIFF
benchmarks.run.windows.x86.checked.mch	+0.01%
benchmarks.run_pgo.windows.x86.checked.mch	+0.01%
benchmarks.run_tiered.windows.x86.checked.mch	+0.01%
coreclr_tests.run.windows.x86.checked.mch	+0.01%
libraries.crossgen2.windows.x86.checked.mch	+0.01%
libraries.pmi.windows.x86.checked.mch	+0.01%
libraries_tests.run.windows.x86.Release.mch	+0.01%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch	+0.01%
realworld.run.windows.x86.checked.mch	+0.01%

FullOpts (+0.00% to +0.01%)

Collection	PDIFF
benchmarks.run_tiered.windows.x86.checked.mch	+0.01%
libraries.crossgen2.windows.x86.checked.mch	+0.01%
libraries.pmi.windows.x86.checked.mch	+0.01%
realworld.run.windows.x86.checked.mch	+0.01%

Details here

davidwrighton · 2024-01-23T18:37:44Z

I've looked at the changes to the crossgen2 and vm codebase, and they appear to be reasonable. I'm not marking this as code reviewed, as I have not reviewed any of the JIT changes, or seen a review from a JIT expert with review privileges.

am11 · 2024-01-24T05:38:29Z

With this change (and a small tweak in perfmap), #96941 also gets unblocked. 👍

shushanhf · 2024-01-24T06:18:34Z

We had published the crossgen2 within our SDK8.0-LoongArch64.
We will push our local pathes to the runtime after the Chinese Spring Festival.

jakobbotsch · 2024-01-24T11:47:47Z

src/coreclr/jit/lsra.cpp

+#ifdef TARGET_RISCV64
+                if (isFloatRegType(type) && genIsValidIntReg(treeNode->GetRegNum()))
+                {
+                    type = (type == TYP_FLOAT) ? TYP_INT : TYP_LONG;
+                }
+#endif // TARGET_RISCV64


In what cases do you have these mismatches in the RISC-V backend? Why can you not get rid of them? (For example by inserting GT_BITCAST)

This PR introduces many RISCV ifdefs in general code because of the mismatch. It would be preferable to avoid it.

When there are too many float arguments to allocate with 8 float argument registers, in RISCV it uses available integer argument registers. So there are mismatches which have float type and integer registers. Could you please share better solutions?

Sorry for making not good enough codes. When I saw errors, I think I tend to focus on fixing errors in RISCV and trying to find easy and simple ways for only RISC-V because of my lack of .NET runtime understanding and developing competency. I am so sorry. And thank you for your help.

No worries, let's try to see if we can avoid them. The idea would be to have lowering insert GT_BITCAST when these mismatches happen. In fact, you can already see that Lowering:LowerFloatArg already has this logic to handle some other cases. Can you generalize Lowering::LowerArg so that it calls LowerFloatArg for these new cases? In the end you would have PUTARG_REG<integer register>(BITCAST<TYP_INT>(TYP_FLOAT node)), instead of the current PUTARG_REG<integer register>(TYP_FLOAT node) that I am assuming that you end up with.

Thanks for your comment. I will try as you told.

I updated mismatch cases based on your comment. I hope it is better. If I misunderstand your comment or it has any problem, please let me know. I will fix it. Thank you!

Looks great to me.

@shushanhf can you please check if LA64 is able to use the same fix so we can remove LA64 specific handling for unspilling as well?

Thanks, I will check it.
We had some patches in our local runtime to release LoongArch64's SDK8.0 and we will push them after the Chinese Spring Festival.
The SDK6.0 and SDK8.0 supporting R2R is OK for LA64, the Intrinsic feature had been finished and is ready to release after the LoongArch64's Instrinsic API merged.

BTW, I think we will push lots of codes during the 2024, liking improving the R2R, supporiting intrinsic and Native-AOT, pushing the mono and some other optimization.
The mono for Unity3D had been finished and the Unity3D game is running on the LoongArch64 linux.

@shushanhf can you please check if LA64 is able to use the same fix so we can remove LA64 specific handling for unspilling as well?

Thanks very much, I think this is OK for LA64.

@shushanhf Thank you for the check. I include a commit for LA64. Thank you.

This reverts commit 381858c.

src/coreclr/vm/riscv64/stubs.cpp

src/coreclr/jit/gentree.h

src/coreclr/jit/lower.cpp

ryujit-bot · 2024-01-29T09:39:36Z

Diff results for #97368

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)

Collection	PDIFF
libraries.pmi.linux.arm64.checked.mch	-0.01%

Details here

clamp03 · 2024-01-30T02:46:18Z

@jakobbotsch I am really appreciate for your help and kind comments. Thank you so much.

@davidwrighton @jakobbotsch reviewed JIT changes and approved. Could you review the other changes and merge if it is good?

clamp03 added 16 commits January 23, 2024 16:42

[RISC-V] Fix crossgen2

d87cfc5

[RISC-V] Fix typo in codegen

6fc0d21

[RISC-V] Update assert to support all rel insts

23fee52

[RISC-V] Fix helper function call

464c170

[RISC-V] Update GetRISCV64PassStructInRegisterFlags

56a39cc

[RISC-V] Fix int arg reg passing for float type

381858c

A failure during crossgen2 SPC.dll `System.Diagnostics.Tracing.NativeRuntimeEventSource:LogThreadPoolWorkerThreadAdjustmentStats(double,double,double,double,double,double,double,double,double,ushort,ushort)`

[RISC-V] Update EPILOG_WITH_TRANSITION_BLOCK_RETURN

73523cf

[RISC-V] Update indentation in dump

07a0f00

[RISC-V] Fix stubs

7b73f4f

[RISC-V] Fix virtualcallstubcpu

d52b5c2

[RISC-V] Update a comment

5d21c03

[RISC-V] Update RO data block

d84ce77

[RISC-V] Fix data overwrites

6e6c457

[RISC-V] Fix typo

60858e0

[RISC-V] Fix register number of A3

00ce57f

[RISC-V] Code Formatting

f575d0a

clamp03 requested a review from MichalStrehovsky as a code owner January 23, 2024 08:07

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 23, 2024

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Jan 23, 2024

tomeksowi approved these changes Jan 23, 2024

View reviewed changes

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

[RISC-V] Fix format and typos

0f9ac89

clamp03 self-assigned this Jan 23, 2024

clamp03 added the arch-riscv Related to the RISC-V architecture label Jan 23, 2024

build-analysis bot mentioned this pull request Jan 23, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

clamp03 requested a review from davidwrighton January 23, 2024 12:00

clamp03 requested review from jakobbotsch and shushanhf January 23, 2024 12:03

jakobbotsch reviewed Jan 24, 2024

View reviewed changes

Revert "[RISC-V] Fix int arg reg passing for float type"

6bc9606

This reverts commit 381858c.

ashaurtaev reviewed Jan 25, 2024

View reviewed changes

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

clamp03 added 2 commits January 25, 2024 19:37

[RISC-V] Fix a typo in a comment

2799636

[RISC-V] Fix error when arg type and arg reg mismatch

541ae4c

jakobbotsch reviewed Jan 26, 2024

View reviewed changes

src/coreclr/jit/gentree.h Outdated Show resolved Hide resolved

jakobbotsch reviewed Jan 26, 2024

View reviewed changes

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

[RISC-V] Rename according to review

7c2b84d

ashaurtaev approved these changes Jan 26, 2024

View reviewed changes

[LoongArch64] Remove LA64 speicific handling for unspilling

0ce2d8c

jakobbotsch approved these changes Jan 29, 2024

View reviewed changes

jakobbotsch merged commit fe51bd7 into dotnet:main Jan 30, 2024
126 of 129 checks passed

am11 mentioned this pull request Jan 30, 2024

Enable riscv64 R2R in installer #96941

Merged

github-actions bot locked and limited conversation to collaborators Feb 29, 2024

clamp03 deleted the crossgen2 branch March 8, 2024 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] Fix errors in crosgen2 for risc-v #97368

[RISC-V] Fix errors in crosgen2 for risc-v #97368

clamp03 commented Jan 23, 2024 •

edited

Loading

clamp03 commented Jan 23, 2024

ryujit-bot commented Jan 23, 2024

Throughput diffs

Throughput diffs for windows/x86 ran on windows/x86

davidwrighton commented Jan 23, 2024

am11 commented Jan 24, 2024

shushanhf commented Jan 24, 2024

jakobbotsch Jan 24, 2024

jakobbotsch Jan 24, 2024

clamp03 Jan 25, 2024

jakobbotsch Jan 25, 2024 •

edited

Loading

clamp03 Jan 25, 2024

clamp03 Jan 26, 2024

jakobbotsch Jan 26, 2024

shushanhf Jan 29, 2024 •

edited

Loading

shushanhf Jan 29, 2024

clamp03 Jan 29, 2024

ryujit-bot commented Jan 29, 2024

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

clamp03 commented Jan 30, 2024

[RISC-V] Fix errors in crosgen2 for risc-v #97368

[RISC-V] Fix errors in crosgen2 for risc-v #97368

Conversation

clamp03 commented Jan 23, 2024 • edited Loading

clamp03 commented Jan 23, 2024

ryujit-bot commented Jan 23, 2024

Throughput diffs

Throughput diffs for windows/x86 ran on windows/x86

davidwrighton commented Jan 23, 2024

am11 commented Jan 24, 2024

shushanhf commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shushanhf Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryujit-bot commented Jan 29, 2024

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

clamp03 commented Jan 30, 2024

clamp03 commented Jan 23, 2024 •

edited

Loading

jakobbotsch Jan 25, 2024 •

edited

Loading

shushanhf Jan 29, 2024 •

edited

Loading