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

Remove __multc3 #623

Closed
wants to merge 1 commit into from
Closed

Remove __multc3 #623

wants to merge 1 commit into from

Conversation

XrXr
Copy link

@XrXr XrXr commented May 27, 2024

This C file doesn't seem to be compiled correctly, as it creates a reference to the x86 specific __builtin_copysignq on ARM. This intrinsic is unused and unavailable on Windows because it's for complex numbers Rust doesn't support, so it should be fine to remove.

The inclusion of the file seems to be the cause of some downstream link errors: rust-lang/rust#125619

This C file doesn't seem to be compiled correctly, as it creates a reference to the x86 specific `__builtin_copysignq` on ARM. This intrinsic is unused and unavailable on Windows because it's for complex numbers Rust doesn't support, so it should be fine to remove.

The inclusion of the file seems to be the cause of some downstream link errors: rust-lang/rust#125619
XrXr added a commit to XrXr/compiler-builtins that referenced this pull request May 28, 2024
@Amanieu
Copy link
Member

Amanieu commented May 28, 2024

This is actually a bug in upstream LLVM that should be fixed there:

#if __has_builtin(__builtin_copysignf128)
#define crt_copysignf128(x, y) __builtin_copysignf128((x), (y))
#elif __has_builtin(__builtin_copysignq) || (defined(__GNUC__) && __GNUC__ >= 7)
#define crt_copysignf128(x, y) __builtin_copysignq((x), (y))
#endif

The condition is incorrect: on GCC 7+, we should be using either __builtin_copysignq or __builtin_copysignf128 depending on the target. From the glibc source I find that x86, ia64 and powerpc use __builtin_copysignq while all other targets use __builtin_copysignf128.

GCC 6 and before simply don't properly support f128.

@XrXr
Copy link
Author

XrXr commented May 29, 2024

Thanks, I was hoping we could fix this just by removing it. I'm surprised that this one slipped through the cracks since any attempt to actually use __multc3 on aarach64 should trigger the link error. I guess upstream LLVM doesn't test the !__has_builtin(__builtin_copysignf128) case, and rust releases build this crate without __builtin_copysignf128 (maybe because of cross compilation?).

But if __builtin_copysignf128 is unavailable, we can't use it in the upstream implementation. 🤔

@Amanieu
Copy link
Member

Amanieu commented May 29, 2024

The issue only happens when compiler-rt is built with GCC, which happens in our build.rs. This is why it was not caught earlier: compiler-rt is normally only built as part of Clang toolchains and clang will always return true for __has_builtin(__builtin_copysignf128).

The best way to fix this would be to send a PR to llvm/llvm-project, and then backport the fix to rust's LLVM fork (rust-lang/llvm-project).

@XrXr
Copy link
Author

XrXr commented May 30, 2024

I'm having trouble recreating the bad object with GCC to make a patch for compiler-rt. I have GCC 13.2, and __has_builtin(__builtin_copysignf128) has been available since GCC 7.1 (1 2). When I make a crate that depends on compiler-builtins with the c feature, it links and runs fine. I'm able to single step into my local copy of __multc3 and everything.
I suspect rust binary releases are built with an old GCC. If they are, a patch for compiler-rt might need to include an implementation of copysignf128 without using the builtin. But also, using a newer GCC might solve this issue without any code change.
I'll poke around to figure out the build environment of binary releases.

@XrXr
Copy link
Author

XrXr commented May 30, 2024

Closing, as I was able to piece enough together to send llvm/llvm-project#93890. Hopefully I get a response. Thanks for the guidance!

@XrXr XrXr closed this May 30, 2024
@XrXr XrXr deleted the rm-multc3 branch May 30, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants