Skip to content

Don't compute FnAbi for LLVM intrinsics - #160077

Open
bjorn3 wants to merge 8 commits into
rust-lang:mainfrom
bjorn3:no_unadjusted_fn_abi
Open

Don't compute FnAbi for LLVM intrinsics#160077
bjorn3 wants to merge 8 commits into
rust-lang:mainfrom
bjorn3:no_unadjusted_fn_abi

Conversation

@bjorn3

@bjorn3 bjorn3 commented Jul 28, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

View all comments

They don't have a sensible FnAbi, so the fact that we still compute an FnAbi for them requires us to make the ABI sanity check more lenient than it should be.

r? @RalfJung as all non-trivial changes are in Miri

@rustbot

rustbot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 28, 2026
@rustbot

rustbot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@bjorn3

bjorn3 commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

I guess this finishes my quest of getting rid of PassMode::Direct for BackendRepr::Memory that spanned over 5 years and 13 PRs:

There is still cleanup that can be done, but at least this wildly wrong combination is gone.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
RalfJung
RalfJung reviewed Jul 28, 2026
View reviewed changes
Comment thread compiler/rustc_const_eval/src/interpret/step.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 commented Jul 29, 2026
View reviewed changes
Comment thread tests/codegen-llvm/const-vector.rs
fn test_f32x2(a: f32x2);
fn test_f32x2_arr(a: f32x2);
fn test_simd(a: Simd<i32, 4>);
fn test_simd_unaligned(a: Simd<i32, 3>);

@bjorn3 bjorn3 Jul 29, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is no way to test this specific case anymore it seems. LLVM doesn't accept PackedSimd on intrinsics, extern "unadjusted" requires LLVM intrinsics and any other ABI doesn't pass non-power-of-2 vectors as { [3 x i32] }.

View changes since the review

@bjorn3
bjorn3 force-pushed the no_unadjusted_fn_abi branch from 7358ce7 to eacedff Compare July 29, 2026 10:58
@bjorn3

bjorn3 commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Looks like compiler-builtins still has some extern "unadjusted" definitions. Will need to replace those.

@bjorn3
bjorn3 force-pushed the no_unadjusted_fn_abi branch 2 times, most recently from ef2938a to e2873a0 Compare July 31, 2026 12:28
@rustbot

rustbot commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

@rustbot rustbot added the A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) label Jul 31, 2026
RalfJung
RalfJung reviewed Aug 1, 2026
View reviewed changes

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interpreter changes mostly LGTM.

compiler-builtins changes will need a review by @tgross35 .

View changes since this review

Comment thread compiler/rustc_const_eval/src/interpret/step.rs Outdated
Comment thread compiler/rustc_const_eval/src/interpret/step.rs Outdated
Comment thread compiler/rustc_monomorphize/src/mono_checks/abi_check.rs
Comment thread compiler/rustc_ty_utils/src/abi.rs
Comment thread tests/codegen-llvm/simd/unpadded-simd.rs Outdated
tgross35
tgross35 reviewed Aug 3, 2026
View reviewed changes
Comment thread library/compiler-builtins/compiler-builtins/src/float/conv.rs
Comment on lines -282 to +310
#[cfg_attr(target_os = "uefi", unadjusted_on_win64)]
#[cfg(not(all(target_os = "uefi", target_arch = "x86_64")))]
pub extern "C" fn __floattisf(i: i128) -> f32 {
int_to_float::signed(i, int_to_float::u128_to_f32_bits)
}

#[cfg_attr(target_os = "uefi", unadjusted_on_win64)]
#[cfg(all(target_os = "uefi", target_arch = "x86_64"))]
pub extern "C" fn __floattisf(lo: u64, hi: u64) -> f32 {
int_to_float::signed((i128::from(hi) << 64) | i128::from(lo), int_to_float::u128_to_f32_bits)
}

#[cfg(not(all(target_os = "uefi", target_arch = "x86_64")))]
pub extern "C" fn __floattidf(i: i128) -> f64 {
int_to_float::signed(i, int_to_float::u128_to_f64_bits)
}

#[cfg(all(target_os = "uefi", target_arch = "x86_64"))]
pub extern "C" fn __floattidf(lo: u64, hi: u64) -> f64 {
int_to_float::signed((i128::from(hi) << 64) | i128::from(lo), int_to_float::u128_to_f64_bits)
}

@tgross35 tgross35 Aug 3, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we even need the special casing anymore? i128 has gone through some ABI changes on Windows and it doesn't look like https://github.com/llvm/llvm-project/blob/41322057c3af16d75e239ec6679c6c2bf7aec157/compiler-rt/lib/builtins/floattisf.c#L28 is doing anything special.

View changes since the review

@bjorn3 bjorn3 Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, they still have different ABIs: https://rust.godbolt.org/z/454nczzdr u128 is passed in xmm0 with extern "C", while u64 + u64 is passed in rdx/rcx just like u128 with extern "unadjusted": https://rust.godbolt.org/z/av1f38KrW

bjorn3 and others added 2 commits August 3, 2026 15:33
@bjorn3 @RalfJung
Add comments
70990da 
Co-authored-by: Ralf Jung <post@ralfj.de>
@bjorn3
Reduce code duplication
0785a67
@RalfJung

RalfJung commented Aug 3, 2026

Copy link
Copy Markdown
Member

@rustbot author
Open discussions are: the test, and compiler-builtins.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
bjorn3 force-pushed the no_unadjusted_fn_abi branch from d2588c6 to a457bee Compare August 4, 2026 13:32
@bjorn3

bjorn3 commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2026
@RalfJung

RalfJung commented Aug 4, 2026

Copy link
Copy Markdown
Member

r=me on the Miri part and the test.
r? @tgross35
for the compiler-builtins part

@rustbot rustbot assigned tgross35 and unassigned RalfJung Aug 4, 2026
@rustbot

rustbot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@tgross35 tgross35 tgross35 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@tgross35 tgross35

Labels

A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bjorn3 @rustbot @rust-log-analyzer @RalfJung @tgross35