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

LLVM is unable to allocate certain function returns on sparc-unknown-linux-gnu #80712

Open
AaronKutch opened this issue Jan 5, 2021 · 8 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-SPARC Target: SPARC processors T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@AaronKutch
Copy link
Contributor

AaronKutch commented Jan 5, 2021

Some time ago, compiler-builtins ran into an issue where the sparc-unknown-linux-gnu target was unable to handle functions that return (u128, u128). I made a temporary workaround in compiler-builtins and said that I would try to fix the underlying issue over winter break. There are currently two proposals for adding functions that return tuples of integers, rust-lang/rfcs#914 and I remember something about widening multiplication that returns a tuple (but I can't find it for some reason). The workaround I made in compiler-builtins will not work in those cases, since the function returning a tuple is directly in the public interface. Since sparc-unknown-linux-gnu is a tier 2.5 target important to some people, we need to fix the issue in LLVM ahead of time or implement some kind of lowering that leads to the same IR that clang generates. Unfortunately, I got distracted by other things and will not have time to dive into LLVM. I am opening this issue so that someone with more experience with LLVM internals can fix the issue. More details are in rust-lang/compiler-builtins#390 and rust-lang/rfcs#914 .

@AaronKutch AaronKutch added the C-bug Category: This is a bug. label Jan 5, 2021
@jyn514 jyn514 added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-SPARC Target: SPARC processors labels Jan 5, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 5, 2021

@AaronKutch is this specific to (u128, u128)? Or is it an issue for all tuples of integers?

@AaronKutch
Copy link
Contributor Author

I don't know because 32-bit SPARC is not a target I can easily download and test from rustup. Arrays and other structures should be tested.

@nagisa
Copy link
Member

nagisa commented Jan 5, 2021

My guess is that the return value ends up getting passed via registers. If that's true, the fix can be as easy as adjusting the ABI code to pass the return value indirectly.

I'm also quite surprised the first thing attempted in working around the issue in compiler-builtins was not changing to a different calling convention.

lowering that leads to the same IR that clang generates

clang's behaviour is irrelevant for "Rust" calling convention. There shouldn't be a goal of matching the behaviour of clang for anything but extern "C" and other well known calling conventions.

@AaronKutch
Copy link
Contributor Author

I know the basics around calling conventions in assembly, but I'm not familiar with the implementation details in Rust and LLVM. Shouldn't LLVM be pushing values onto the stack instead of giving up when it realizes that the registers cannot hold all the values? I can define a function
pub fn test(x: u128) -> (u128, u128, u128, u128, u128, u128, u128, u128, u128, u128, u128, u128, u128, u128) and it will compile just fine on sparc64. I can look at the assembly and see it moving a bunch of values to the stack. There should be no problems except for running out of memory for the stack at runtime or the program not being able to be loaded into memory. There might be some special things surrounding extern functions that I don't know about, but the failing return allocations are happening on a perfectly normal Rust signature fn f(x: u128, y: u128) -> (u128, u128).

What I mean by lowering to the same IR that clang generates is that Rust's IR relies on LLVM dealing with it:

ret { i128, i128 } %1

clang's IR explicitly puts the value on the stack for LLVM:

store i128 2, i128* %3, align 16, !tbaa !7
ret void

So this would be a way of fully supporting sparc-unknown-linux-gnu until the underlying LLVM issue is fixed on all supported LLVM versions.

I'm also quite surprised the first thing attempted in working around the issue in compiler-builtins was not changing to a different calling convention.

What do you mean by changing calling convention? The only way of forcing custom calling conventions is to use assembly in #[naked] functions. We don't want to use very weird hacks if we add functions of the form fn f(x: u128, y: u128) -> (u128, u128) and also want to support sparc-unkown-linux-gnu.

@nagisa
Copy link
Member

nagisa commented Jan 5, 2021

What do you mean by changing calling convention?

Well, if the functions in question are internal implementation details, you can mark them as extern "C"

extern "C" fn test_128(one: u128, two: u128) -> (u128, u128) { ... }

which should in principle give you the same ABI (and LLVM IR) as what clang uses in the first place.

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Jan 5, 2021

I suppose that would have also worked, but still an ugly workaround for just one edge case, the root of which should be fixed. I should mention that the choice I made in compiler-builtins is more of a permanent workaround. SPARC is an awful architecture to optimize several classes of algorithms for because it does not have widening multiplication. Widening multiplication is very important for the performance of the default division algorithm choices made in compiler-builtins. Originally, I didn't want to waste any time on an architecture I didn't want to care about, but when the 32 bit sparc issue came around I decided to knock out two birds with one stone and make a special codepath for all SPARC architectures.

@nagisa
Copy link
Member

nagisa commented Jan 8, 2021

Filled bugs for the LLVM asserts upstream https://bugs.llvm.org/show_bug.cgi?id=48698 and https://bugs.llvm.org/show_bug.cgi?id=48699 but we should still adjust ABI code in Rust.

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@Skgland
Copy link
Contributor

Skgland commented Feb 19, 2024

Filled bugs for the LLVM asserts upstream https://bugs.llvm.org/show_bug.cgi?id=48698 and https://bugs.llvm.org/show_bug.cgi?id=48699 but we should still adjust ABI code in Rust.

48698 appears to have been migrated to Github as llvm/llvm-project#48042 and has been closed
48699 appears to have been migrated to Github as llvm/llvm-project#48043 and is still open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-SPARC Target: SPARC processors 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