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 some unused rust specific intrinsics #745

Closed
wants to merge 2 commits into from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 9, 2025

I think they were originally introduced when 128bit int support was added to rustc but before LLVM consistently supported 128bit int operations across platforms. Nowadays LLVM and GCC handle 128bit int operations for us rather than manually adding calls to rust specific intrinsics. The only exception is the Cranelift backend which still uses the __rust_u/i128_mulo intrinsics and as such this PR keeps those around.

I think they were originally introduced when 128bit int support was
added to rustc but before LLVM consistently supported 128bit int
operations across platforms. Nowadays LLVM and GCC handle 128bit int
operations for us rather than manually adding calls to
rust specific intrinsics. The only exception is the Cranelift backend
which still uses the __rust_u/i128_mulo intrinsics and as such this
commit keeps those around.
This prevents rust-analyzer from showing an error for the panic-handler
crate.
@bjorn3 bjorn3 force-pushed the remove_unused_intrinsics branch from e6d27ad to 287c279 Compare January 9, 2025 12:38
@tgross35
Copy link
Contributor

tgross35 commented Jan 9, 2025

From a quick look it seems like gc_gcc may still use these https://github.com/rust-lang/rust/blob/824759493246ee383beb9cd5ceffa0e15deb9fa4/compiler/rustc_codegen_gcc/src/int.rs#L180-L183 https://github.com/rust-lang/rust/blob/824759493246ee383beb9cd5ceffa0e15deb9fa4/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs#L988, but maybe is_native* now return true so those codepaths don't get hit anymore?

Cc @rust-lang/wg-gcc-backend

@antoyo
Copy link

antoyo commented Jan 9, 2025

GCC only supports the type __int128 on some platforms as far as I know. But it might be possible to use the equivalent of C23's _BitInt now and stop using those intrinsics.

@tgross35
Copy link
Contributor

tgross35 commented Jan 9, 2025

GCC only supports the type __int128 on some platforms as far as I know. But it might be possible to use the equivalent of C23's _BitInt now and stop using those intrinsics.

As in, using _BitInt only when __int128 is not available? That sounds reasonable. Unfortunately you probably can't use _BitInt(128) all the time, since it is not ABI-compatible with __int128 on x86 (which IMO is a terrible design decision).

I guess in either case we should probably keep these around at least for a little while.

@bjorn3 bjorn3 closed this Jan 9, 2025
@bjorn3 bjorn3 deleted the remove_unused_intrinsics branch January 10, 2025 08:50
@antoyo
Copy link

antoyo commented Jan 11, 2025

As in, using _BitInt only when __int128 is not available?

Yes.

Unfortunately you probably can't use _BitInt(128) all the time, since it is not ABI-compatible with __int128 on x86 (which IMO is a terrible design decision).

Thanks for the info. I took note of that in the issue.

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