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

Disable f128 for amdgpu #737

Merged
merged 2 commits into from
Dec 26, 2024
Merged

Disable f128 for amdgpu #737

merged 2 commits into from
Dec 26, 2024

Conversation

Flakebi
Copy link
Contributor

@Flakebi Flakebi commented Dec 25, 2024

compiler_builtins fails to compile to amdgpu if f128 is enabled. The reason seems to be that compiler_builtins uses libcalls in the implementation. I’m not really familiar with what libcalls are, but the LLVM amdgpu backend explicitly does not support them.

Error message:

LLVM ERROR: unsupported libcall legalization

(The amdgpu target is not yet supported in rustc, there is an open PR here: rust-lang/rust#134740)

`compiler_builtins` fails to compile to amdgpu if f128 is enabled.
The reason seems to be that compiler_builtins uses libcalls in the
implementation. I’m not really familiar with what libcalls are, but the
LLVM amdgpu backend explicitly does not support them.

Error message:
```
LLVM ERROR: unsupported libcall legalization
```
@tgross35
Copy link
Contributor

Looks reasonable to me, would you be able to file an LLVM issue and add a link in the comment?

@Flakebi
Copy link
Contributor Author

Flakebi commented Dec 26, 2024

Looks reasonable to me, would you be able to file an LLVM issue and add a link in the comment?

Done!

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@tgross35 tgross35 merged commit fe6b093 into rust-lang:master Dec 26, 2024
26 checks passed
@github-actions github-actions bot mentioned this pull request Dec 26, 2024
@tgross35
Copy link
Contributor

After #731 merges, you can submit a PR to rust-lang/rust to update to 0.1.140. Feel free to r? me there.

@tgross35
Copy link
Contributor

tgross35 commented Dec 26, 2024

Also @Flakebi if you get the chance, could you post the whole error message / backtrace here? I’m curious where exactly this is failing. I have to expect it’s something like multiplication routines not being able to call addition on f128 (which should just be a libcall to addtf3, also provided in this crate).

I’m not really familiar with what libcalls are

For context, basic operations (add, subtract, etc), intrinsics (in Rust) and builtins (name for intrinsics in C) get sent to the backend as something like f128 c = add a, b. LLVM has the choice to either lower it to an assembly routine or a call to a library function that it expects to be available. Most integers and f32/f64 will be lowered to assembly, but most platforms don’t have hardware f128 so that turns into a “libcall” with a symbol name like __addtf3, __multf3, etc (tf=tetrafloat=f128, don’t ask). These symbols just need to be available when linking, and for Rust it’s this crate that provides them.

I’m not an LLVM expert but I think the error is just saying that it’s trying to turn something like f128 + f128 into a libcall since there is no asm routine, but it hasn’t been taught it can do this by calling __addtf3.

@Flakebi Flakebi deleted the amdgpu branch December 26, 2024 11:26
tgross35 added a commit to tgross35/rust that referenced this pull request Dec 27, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Dec 27, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Dec 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2024
Rollup merge of rust-lang#134832 - tgross35:update-builtins, r=tgross35

Update `compiler-builtins` to 0.1.140

Nothing significant here, just syncing the following small changes:

- rust-lang/compiler-builtins#727
- rust-lang/compiler-builtins#730
- rust-lang/compiler-builtins#736
- rust-lang/compiler-builtins#737
poliorcetics pushed a commit to poliorcetics/rust that referenced this pull request Dec 28, 2024
poliorcetics pushed a commit to poliorcetics/rust that referenced this pull request Dec 28, 2024
@Flakebi
Copy link
Contributor Author

Flakebi commented Dec 29, 2024

I think I made a slight mistake and should have checked for amdgcn instead of amdgpu.
I’ll fix this once the name is final in the Rust PR.

if you get the chance, could you post the whole error message / backtrace here? I’m curious where exactly this is failing.

It fails to compile here, because that’s the (only?) place where compiler-builtins uses a builtin itself:

#[cfg(f128_enabled)]
// FIXME(f16_f128): MSVC cannot build these until `__divtf3` is available in nightly.
#[cfg(not(target_env = "msvc"))]
pub extern "C" fn __powitf2(a: f128, b: i32) -> f128 {
pow(a, b)
}

More details about why this fails are in llvm/llvm-project#121122

@bjorn3
Copy link
Member

bjorn3 commented Dec 29, 2024

The __powitf2 function shouldn't be compiled at all if f128 is correctly disabled.

@Flakebi
Copy link
Contributor Author

Flakebi commented Dec 29, 2024

The __powitf2 function shouldn't be compiled at all if f128 is correctly disabled.

Right, compiler-builtins compiles correctly (without __powitf) if f128 is disabled :)

(Sorry, I mixed two points in the previous comment, (1) about the arch name and (2) about your previous question about where exactly it failed without disabling f128.)

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.

3 participants