-
Notifications
You must be signed in to change notification settings - Fork 924
fix(LLVM,riscv64): fix calling conventions for i32 type
#6072
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request fixes calling conventions for the i32 type on RISC-V 64-bit platforms to comply with the RISC-V SysV ABI requirement that all 32-bit values must be held in sign-extended format in 64-bit registers.
Changes:
- Introduces a new RISC-V SystemV ABI implementation with proper handling of
i32sign-extension requirements - Adds
signextandnoundefattributes to alli32parameters in function declarations and call sites for RISC-V 64-bit targets - Refactors common ABI functionality into default trait implementations to reduce code duplication
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/compiler-llvm/src/abi/riscv_systemv.rs | New file implementing RISC-V SystemV ABI with i32 sign-extension handling |
| lib/compiler-llvm/src/abi/mod.rs | Refactors common ABI methods into default trait implementations; adds RISC-V ABI selection |
| lib/compiler-llvm/src/abi/x86_64_systemv.rs | Removes duplicate code now in default trait implementations |
| lib/compiler-llvm/src/abi/aarch64_systemv.rs | Removes duplicate code now in default trait implementations |
| lib/compiler-llvm/src/translator/intrinsics.rs | Adds target_triple parameter and applies i32 attributes to intrinsic functions for RISC-V |
| lib/compiler-llvm/src/translator/code.rs | Adds build_call_with_param_attributes helper to apply i32 attributes at call sites for RISC-V |
| lib/compiler-llvm/src/trampoline/wasm.rs | Updates Intrinsics::declare calls with new target_triple parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (i, param) in ty.get_param_types().iter().enumerate() { | ||
| if param == &i32_ty_basic_md { | ||
| function.add_attribute( | ||
| AttributeLoc::Param(i as u32), | ||
| context.create_enum_attribute( | ||
| Attribute::get_named_enum_kind_id("signext"), | ||
| 0, | ||
| ), | ||
| ); | ||
| function.add_attribute( | ||
| AttributeLoc::Param(i as u32), | ||
| context.create_enum_attribute( | ||
| Attribute::get_named_enum_kind_id("noundef"), | ||
| 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.
I find a bit strange that this logic lives here, it feels a bit of a hack.
Do you think this could be improved?
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.
Good point! We can do better by using the https://thedan64.github.io/inkwell/inkwell/intrinsics/struct.Intrinsic.html#method.find API, that should give us a properly annotated declaration that hopefully takes target ABI into account.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hey thanks for this change! I have run all the tests on our side using Rust 1.92.0, and they all pass. So as far as our tests, this change did resolve the bug in #5977 |
|
Cool, thanks for the confirmation. |
There's a special requirement for the RISC-V SysV ABI:
https://five-embeddev.com/riscv-user-isa-manual/Priv-v1.12/rv64.html
So far, we've been using
X86_64SystemVfor the RISC-V ABI, and so I copied it into a separate one, where extra param attributes are added. That's something we must address as part of #5943. On top of that I decided to put some common functionality into theAbitrait itself.@wakabat can you give it a try?
Fixes: #5977