Skip to content
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

riscv codegen oddity: emits (hopefully no-op) signext for a u32 param. #98151

Closed
pnkfelix opened this issue Jun 15, 2022 · 7 comments
Closed
Labels
A-codegen Area: Code generation A-ffi Area: Foreign Function Interface (FFI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

I tried this code:

// compile-flags: --target riscv64gc-unknown-none-elf -C no-prepopulate-passes --emit=llvm-ir

#![feature(no_core, lang_items)]
#![crate_type = "lib"]
#![no_std]
#![no_core]

#[lang="sized"] trait Sized { }
#[lang="freeze"] trait Freeze { }
#[lang="copy"] trait Copy { }

#[no_mangle] pub extern "C" fn c_arg_u8(_a: u8) { }
#[no_mangle] pub extern "C" fn c_arg_u16(_a: u16) { }
#[no_mangle] pub extern "C" fn c_arg_u32(_a: u32) { }
#[no_mangle] pub extern "C" fn c_arg_u64(_a: u64) { }

#[no_mangle] pub extern "C" fn c_ret_u8() -> u8 { 0 }
#[no_mangle] pub extern "C" fn c_ret_u16() -> u16 { 0 }
#[no_mangle] pub extern "C" fn c_ret_u32() -> u32 { 0 }
#[no_mangle] pub extern "C" fn c_ret_u64() -> u64 { 0 }

#[no_mangle] pub extern "C" fn c_ret_i8() -> i8 { 0 }
#[no_mangle] pub extern "C" fn c_ret_i16() -> i16 { 0 }
#[no_mangle] pub extern "C" fn c_ret_i32() -> i32 { 0 }
#[no_mangle] pub extern "C" fn c_ret_i64() -> i64 { 0 }

Note the compile flags; thus I invoked the compiler like so:

% rustc +nightly --target riscv64gc-unknown-none-elf -C no-prepopulate-passes demo.rs --emit=llvm-ir

I expected to see this happen:

Semi-uniform handling of unsigned and signed extension of the arguments/return values. Namely, I would expect uN types to be (at most) zero-extended, and iN types to be (at most) signed-extended.

Instead, this happened: The u32 handling emits a signext attribute, like so:


; Function Attrs: nounwind
define dso_local void @c_arg_u32(i32 signext %_a) unnamed_addr #0 {
start:
  ret void
}

...

; Function Attrs: nounwind
define dso_local signext i32 @c_ret_u32() unnamed_addr #0 {
start:
  ret i32 0
}

I think this ends up acting as having no effect, but its weird nonetheless to have it there.

@pnkfelix pnkfelix added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation A-ffi Area: Foreign Function Interface (FFI) O-riscv Target: RISC-V architecture I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 15, 2022
@pnkfelix
Copy link
Member Author

(my current guess is that this supposedly incorrect signext attribute actually has no effect and thus this issue is super-low priority. but it would be nice to get independent confirmation of that guess...)

@pnkfelix
Copy link
Member Author

(this was spawned off my attempt to craft a more general test case for #97800)

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jun 16, 2022

In RV64 all 32-bit operations will be sign-extend to 64-bit, unlike other platforms. (The intention being that this will 64-bit comparison also work directly on 32-bit values, so no need for additional branch instructions)

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jun 16, 2022

Unsigned 8-bit and 16-bit values are essentially zero-extended into 32-bit values (regardless RV32 or RV64), and then in RV64 the zero-extended value is then sign-extended into 64-bit, which effectively make them being zero-extended directly into 64-bit.

EDIT: Some background, in RV64 there are native instructions operating on 32-bit values, but neither RV32/64 have native instructions dealing with 8 or 16-bit values.

@nbdd0121
Copy link
Contributor

So the current behaviour is correct and expected.

@apiraino
Copy link
Contributor

As per the last comment, removing priority from this issue (Zulip discussion).

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 22, 2022
@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 3, 2022

In spite of the popular rumor that LLVM's instruction set is "architecture-neutral", there are quite a few architecture-specific variations in the "LLVM ISA".

But some things do have an architecture-neutral implication, even when they have architecture-specific meaning. In particular, the signext attribute is a parameter attribute that only insists on extension to the degree specified by the target ABI. Thus, if no sign extension is mandated according to the target ABI, it is a no-op. Between that and nbdd's explanation, we seem to have confirmed more-or-less exactly your hypothesis!

Thank you for reporting! It seems this is notabug, so I am closing this.

@workingjubilee workingjubilee removed the C-bug Category: This is a bug. label Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-ffi Area: Foreign Function Interface (FFI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants