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.

Sign up for GitHub

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

Jump to bottom

Ensure shift instrinsic arguments match width of compiler-rt's. #522

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Ensure shift instrinsic arguments match width of compiler-rt's. #522

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/int/shift.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ intrinsics! {
#[avr_skip]
#[maybe_use_optimized_c_shim]
#[arm_aeabi_alias = __aeabi_llsl]
pub extern "C" fn __ashldi3(a: u64, b: u32) -> u64 {
a.ashl(b)
pub extern "C" fn __ashldi3(a: u64, b: core::ffi::c_uint) -> u64 {
a.ashl(b as u32)
Comment on lines +81 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by comment:

While this appears to follow the calling convention that LLVM uses, it does not follow the calling convention that avr-gcc uses: https://godbolt.org/z/azq4749hd
b is only a single byte on avr-gcc.
(I also see now that the C implementation in compiler-rt has the same bug, but only realized it now).

Copy link
Contributor Author

@cr1901 cr1901 Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AIUI, these code paths are skipped when compiling for avr, precisely because there's a pass which gets rid of these types of shifts in LLVM. So I made no accommodations for its nonstandard calling conventions in this PR.

That being said, I don't know what a proper fix should look like that fixes avr, msp430, and everything-else.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because there's a pass which gets rid of these types of shifts in LLVM

I know, because I wrote that pass ;)
But it only works for 32-bit values.__ashldi3 is a 64-bit shift function, which is emitted as a regular compiler-rt call. Proof: https://godbolt.org/z/zqWfE6ndq

Anyway, it doesn't really matter as LLVM uses i16 and not i8 like avr-gcc does. This PR would only be buggy for avr-gcc, right now it is safe with LLVM (but this could in theory change as i8 is more efficient).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minddump for future-me follows:

b is only a single byte on avr-gcc.

  • According to gcc docs, __ashldi3's b argument is supposed to take an int, which must be at least 16 bits. But indeed, your GCC code snippet is only sending a byte to __ashldi3, while the LLVM version sends 16 bits (the clr r17 line is clearing the top-most bits).
  • On the other hand, gcc's docs also says __ashldi3 is supposed to return a long, and __ashlti3 is the function that is supposed to return a long long.
  • On the other _other hand, compiler-rt doesn't seem to support the t versions of functions, and tends to hardcode a 64/32-bit int in places gcc's builtins do not1. In addition to compiler-rt uses unsigned in args where gcc's builtins are signed.

In other words, it seems like both gcc compiler-rt builtins tend to do their own things and aren't directly compatible with each other. avr using a byte in for the gcc __ashldi3 b arg, but 2 bytes for compiler-rt's __ashldi3 b arg is just one example of this. It's not going to be fun :P. Unifying compiler-rt's and gcc's builtins in terms of compiler_builtins might be necessary due to the gcc backend though.

  1. The shift functions' b args notwithstanding :P.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it only works for 32-bit values. __ashldi3 is a 64-bit shift function, which is emitted as a regular compiler-rt call.

How does this not break for Rust code, where all the shift functions are skipped for AVR (avr_skip)?

}

#[avr_skip]
Expand All @@ -96,8 +96,8 @@ intrinsics! {
#[avr_skip]
#[maybe_use_optimized_c_shim]
#[arm_aeabi_alias = __aeabi_lasr]
pub extern "C" fn __ashrdi3(a: i64, b: u32) -> i64 {
a.ashr(b)
pub extern "C" fn __ashrdi3(a: i64, b: core::ffi::c_uint) -> i64 {
a.ashr(b as u32)
}

#[avr_skip]
Expand All @@ -114,8 +114,8 @@ intrinsics! {
#[avr_skip]
#[maybe_use_optimized_c_shim]
#[arm_aeabi_alias = __aeabi_llsr]
pub extern "C" fn __lshrdi3(a: u64, b: u32) -> u64 {
a.lshr(b)
pub extern "C" fn __lshrdi3(a: u64, b: core::ffi::c_uint) -> u64 {
a.lshr(b as u32)
}

#[avr_skip]
Expand Down