-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Make linux-riscv nativeaot port robust #112736
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Filip Navara <filip.navara@gmail.com>
…city guarantees of RhpCheckedXchg
…ount (to be reviewed)
…tion and PROLOG_SAVE_REG_PAIR_NO_FP_INDEXED
RISC-V Release-CLR-VF2: 9461 / 9537 (99.20%)
Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-VF2: 633247 / 669869 (94.53%)
Build information and commandsGIT: RISC-V Release-CLR-QEMU: 9461 / 9537 (99.20%)
Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-QEMU: 657661 / 687860 (95.61%)
Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz Build information and commandsGIT: |
RISC-V Release-CLR-VF2: 9461 / 9537 (99.20%)
Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-CLR-QEMU: 9461 / 9537 (99.20%)
Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-VF2: 626100 / 666185 (93.98%)
Build information and commandsGIT: |
...er.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunGenericHelperNode.cs
Outdated
Show resolved
Hide resolved
encoder.EmitADDI(encoder.TargetRegister.Arg3, encoder.TargetRegister.Arg0, -NonGCStaticsNode.GetClassConstructorContextSize(factory.Target)); | ||
encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg3, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encoder.EmitADDI(encoder.TargetRegister.Arg3, encoder.TargetRegister.Arg0, -NonGCStaticsNode.GetClassConstructorContextSize(factory.Target)); | |
encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg3, 0); | |
encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg0, -NonGCStaticsNode.GetClassConstructorContextSize(factory.Target)); |
ld
can take the same offset, saving one instruction. The address in a3
doesn't look to be read from elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a3
is used on line 82 (to second arg of void EmitMOV(Register regDst, Register regSrc)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. The EmitMOV could turn into EmitADDI with the same offset, though:) Doesn't matter much either way.
I applied latest suggestions and running into sigabort. I haven't debugged it but pushed the patch to a separate branch: am11/runtime@feature/nativeaot/riscv64-port...feature/nativeaot/riscv64-port2 @tomeksowi, can we do it so in this round we focus on correctness and use follow-up PR for further optimizations? This PR is only achieving the next milestone "it passes smoke tests", it's not the final one in the series. We still have ways to go; libs testing (AOT mode), different configuration testing (e.g. release build, workstation vs. server GC etc.). |
Agreed. |
I get a crash even with the currently applied changes in this PR:
I did a full rebuild of everything to rule out any outdated binaries. |
I probably caused that one myself, sorry. Seems like I forgot to save a file in VS Code :-/ |
RISC-V Release-CLR-VF2: 9461 / 9537 (99.20%)
Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz Build information and commandsGIT: |
Co-authored-by: Tomek Sowiński <tomeksowi@gmail.com>
No worries, it happened to me as well (especially when web version of VS Code saves automatically while desktop version requires Command+S 🙂). Do you see anything additional we missed in am11/runtime@feature/nativeaot/riscv64-port...feature/nativeaot/riscv64-port2 (possibly in |
Yes. #112736 (comment) |
@am11 I pushed changes addressing all the feedback save for #112736 (comment). Tested on device. |
2fd845a is being scheduled for building and testingGIT: |
RISC-V Release-CLR-VF2: 9461 / 9537 (99.20%)
Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-QEMU: 627597 / 655412 (95.76%)
Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-VF2: 622743 / 658502 (94.57%)
Build information and commandsGIT: RISC-V Release-CLR-QEMU: 9461 / 9537 (99.20%)
Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz Build information and commandsGIT: |
Thank you! Tested after a clean build, haven't found any issue. 👍 |
0acb29d is being scheduled for building and testingGIT: Release-build FAILEDbuildinfo.json |
Huge thanks to @filipnavara, all smoke tests are passing (except DwarfDump which currently doesn't account for per-architecture stats).
Contributes to #106223. We will run AOT libs tests next.