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

Dellvmize some intrinsics (use u32 instead of Self in some integer intrinsics) #124003

Merged
merged 7 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/example/example.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ pub fn array_as_slice(arr: &[u8; 3]) -> &[u8] {
arr
}

pub unsafe fn use_ctlz_nonzero(a: u16) -> u16 {
pub unsafe fn use_ctlz_nonzero(a: u16) -> u32 {
intrinsics::ctlz_nonzero(a)
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/example/mini_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ pub mod intrinsics {
pub fn min_align_of_val<T: ?::Sized>(val: *const T) -> usize;
pub fn copy<T>(src: *const T, dst: *mut T, count: usize);
pub fn transmute<T, U>(e: T) -> U;
pub fn ctlz_nonzero<T>(x: T) -> T;
pub fn ctlz_nonzero<T>(x: T) -> u32;
#[rustc_safe_intrinsic]
pub fn needs_drop<T: ?::Sized>() -> bool;
#[rustc_safe_intrinsic]
Expand Down
10 changes: 7 additions & 3 deletions compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use rustc_span::source_map::Spanned;
use rustc_span::symbol::{sym, Symbol};

pub(crate) use self::llvm::codegen_llvm_intrinsic_call;
use crate::cast::clif_intcast;
use crate::prelude::*;

fn bug_on_incorrect_arg_count(intrinsic: impl std::fmt::Display) -> ! {
Expand Down Expand Up @@ -627,7 +628,8 @@ fn codegen_regular_intrinsic_call<'tcx>(

// FIXME trap on `ctlz_nonzero` with zero arg.
let res = fx.bcx.ins().clz(val);
let res = CValue::by_val(res, arg.layout());
let res = clif_intcast(fx, res, types::I32, false);
let res = CValue::by_val(res, ret.layout());
ret.write_cvalue(fx, res);
}
sym::cttz | sym::cttz_nonzero => {
Expand All @@ -636,15 +638,17 @@ fn codegen_regular_intrinsic_call<'tcx>(

// FIXME trap on `cttz_nonzero` with zero arg.
let res = fx.bcx.ins().ctz(val);
let res = CValue::by_val(res, arg.layout());
let res = clif_intcast(fx, res, types::I32, false);
let res = CValue::by_val(res, ret.layout());
ret.write_cvalue(fx, res);
}
sym::ctpop => {
intrinsic_args!(fx, args => (arg); intrinsic);
let val = arg.load_scalar(fx);

let res = fx.bcx.ins().popcnt(val);
let res = CValue::by_val(res, arg.layout());
let res = clif_intcast(fx, res, types::I32, false);
let res = CValue::by_val(res, ret.layout());
ret.write_cvalue(fx, res);
}
sym::bitreverse => {
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_gcc/example/example.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,10 @@ fn array_as_slice(arr: &[u8; 3]) -> &[u8] {
arr
}

unsafe fn use_ctlz_nonzero(a: u16) -> u16 {
intrinsics::ctlz_nonzero(a)
}
// FIXME: fix the intrinsic implementation to work with the new ->u32 signature
// unsafe fn use_ctlz_nonzero(a: u16) -> u32 {
// intrinsics::ctlz_nonzero(a)
// }

fn ptr_as_usize(ptr: *const u8) -> usize {
ptr as usize
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/example/mini_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ pub mod intrinsics {
pub fn min_align_of_val<T: ?Sized>(val: *const T) -> usize;
pub fn copy<T>(src: *const T, dst: *mut T, count: usize);
pub fn transmute<T, U>(e: T) -> U;
pub fn ctlz_nonzero<T>(x: T) -> T;
pub fn ctlz_nonzero<T>(x: T) -> u32;
#[rustc_safe_intrinsic]
pub fn needs_drop<T: ?Sized>() -> bool;
#[rustc_safe_intrinsic]
Expand Down
27 changes: 19 additions & 8 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,25 +315,32 @@ impl<'ll, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'_, 'll, 'tcx> {
Some((width, signed)) => match name {
sym::ctlz | sym::cttz => {
let y = self.const_bool(false);
self.call_intrinsic(
let ret = self.call_intrinsic(
&format!("llvm.{name}.i{width}"),
&[args[0].immediate(), y],
)
);

self.intcast(ret, llret_ty, false)
}
sym::ctlz_nonzero => {
let y = self.const_bool(true);
let llvm_name = &format!("llvm.ctlz.i{width}");
self.call_intrinsic(llvm_name, &[args[0].immediate(), y])
let ret = self.call_intrinsic(llvm_name, &[args[0].immediate(), y]);
self.intcast(ret, llret_ty, false)
}
sym::cttz_nonzero => {
let y = self.const_bool(true);
let llvm_name = &format!("llvm.cttz.i{width}");
self.call_intrinsic(llvm_name, &[args[0].immediate(), y])
let ret = self.call_intrinsic(llvm_name, &[args[0].immediate(), y]);
self.intcast(ret, llret_ty, false)
}
sym::ctpop => {
let ret = self.call_intrinsic(
&format!("llvm.ctpop.i{width}"),
&[args[0].immediate()],
);
self.intcast(ret, llret_ty, false)
}
sym::ctpop => self.call_intrinsic(
&format!("llvm.ctpop.i{width}"),
&[args[0].immediate()],
),
sym::bswap => {
if width == 8 {
args[0].immediate() // byte swap a u8/i8 is just a no-op
Expand All @@ -355,6 +362,10 @@ impl<'ll, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'_, 'll, 'tcx> {
// rotate = funnel shift with first two args the same
let llvm_name =
&format!("llvm.fsh{}.i{}", if is_left { 'l' } else { 'r' }, width);

// llvm expects shift to be the same type as the values, but rust always uses `u32`
let raw_shift = self.intcast(raw_shift, self.val_ty(val), false);
scottmcm marked this conversation as resolved.
Show resolved Hide resolved

self.call_intrinsic(llvm_name, &[val, val, raw_shift])
}
sym::saturating_add | sym::saturating_sub => {
Expand Down
31 changes: 21 additions & 10 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let ty = instance_args.type_at(0);
let layout = self.layout_of(ty)?;
let val = self.read_scalar(&args[0])?;
let out_val = self.numeric_intrinsic(intrinsic_name, val, layout)?;

let out_val = self.numeric_intrinsic(intrinsic_name, val, layout, dest.layout)?;
self.write_scalar(out_val, dest)?;
}
sym::saturating_add | sym::saturating_sub => {
Expand All @@ -200,21 +201,24 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
sym::rotate_left | sym::rotate_right => {
// rotate_left: (X << (S % BW)) | (X >> ((BW - S) % BW))
// rotate_right: (X << ((BW - S) % BW)) | (X >> (S % BW))
let layout = self.layout_of(instance_args.type_at(0))?;
let layout_val = self.layout_of(instance_args.type_at(0))?;
let val = self.read_scalar(&args[0])?;
let val_bits = val.to_bits(layout.size)?;
let val_bits = val.to_bits(layout_val.size)?;

let layout_raw_shift = self.layout_of(self.tcx.types.u32)?;
let raw_shift = self.read_scalar(&args[1])?;
let raw_shift_bits = raw_shift.to_bits(layout.size)?;
let width_bits = u128::from(layout.size.bits());
let raw_shift_bits = raw_shift.to_bits(layout_raw_shift.size)?;

let width_bits = u128::from(layout_val.size.bits());
let shift_bits = raw_shift_bits % width_bits;
let inv_shift_bits = (width_bits - shift_bits) % width_bits;
let result_bits = if intrinsic_name == sym::rotate_left {
(val_bits << shift_bits) | (val_bits >> inv_shift_bits)
} else {
(val_bits >> shift_bits) | (val_bits << inv_shift_bits)
};
let truncated_bits = self.truncate(result_bits, layout);
let result = Scalar::from_uint(truncated_bits, layout.size);
let truncated_bits = self.truncate(result_bits, layout_val);
let result = Scalar::from_uint(truncated_bits, layout_val.size);
self.write_scalar(result, dest)?;
}
sym::copy => {
Expand Down Expand Up @@ -472,6 +476,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
name: Symbol,
val: Scalar<M::Provenance>,
layout: TyAndLayout<'tcx>,
ret_layout: TyAndLayout<'tcx>,
) -> InterpResult<'tcx, Scalar<M::Provenance>> {
assert!(layout.ty.is_integral(), "invalid type for numeric intrinsic: {}", layout.ty);
let bits = val.to_bits(layout.size)?;
Expand All @@ -483,11 +488,17 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
sym::ctlz | sym::ctlz_nonzero => u128::from(bits.leading_zeros()) - extra,
sym::cttz | sym::cttz_nonzero => u128::from((bits << extra).trailing_zeros()) - extra,
sym::bswap => (bits << extra).swap_bytes(),
sym::bitreverse => (bits << extra).reverse_bits(),
sym::bswap => {
assert_eq!(layout, ret_layout);
(bits << extra).swap_bytes()
}
sym::bitreverse => {
assert_eq!(layout, ret_layout);
(bits << extra).reverse_bits()
}
_ => bug!("not a numeric intrinsic: {}", name),
};
Ok(Scalar::from_uint(bits_out, layout.size))
Ok(Scalar::from_uint(bits_out, ret_layout.size))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it can ICE when bswap/bitrevere is called with a too small ret_layout.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that happen though? The intrinsics are defined like pub fn bitreverse<T: Copy>(x: T) -> T;, so the ret_layout must be the same?

Copy link
Member

Choose a reason for hiding this comment

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

That's an extremely non-local invariant. This function here should make sense on its own without worrying about what happens in the standard library.

Copy link
Member

@RalfJung RalfJung Apr 16, 2024

Choose a reason for hiding this comment

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

Maybe add an assertion that for bswap/bitreverse, the two layouts are the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is true in general with intrinsics, we implicitly depend on their signature in a lot of ways. Either way, I added an assertion.

Copy link
Member

Choose a reason for hiding this comment

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

In the interpreter and Miri we always have local assertions for these things.

}

pub fn exact_div(
Expand Down
14 changes: 6 additions & 8 deletions compiler/rustc_hir_analysis/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,13 +411,11 @@ pub fn check_intrinsic_type(
(1, 0, vec![Ty::new_mut_ptr(tcx, param(0)), param(0)], Ty::new_unit(tcx))
}

sym::ctpop
| sym::ctlz
| sym::ctlz_nonzero
| sym::cttz
| sym::cttz_nonzero
| sym::bswap
| sym::bitreverse => (1, 0, vec![param(0)], param(0)),
sym::ctpop | sym::ctlz | sym::ctlz_nonzero | sym::cttz | sym::cttz_nonzero => {
(1, 0, vec![param(0)], tcx.types.u32)
}

sym::bswap | sym::bitreverse => (1, 0, vec![param(0)], param(0)),

sym::three_way_compare => {
(1, 0, vec![param(0), param(0)], tcx.ty_ordering_enum(Some(span)))
Expand Down Expand Up @@ -460,7 +458,7 @@ pub fn check_intrinsic_type(
(1, 0, vec![param(0), param(0)], param(0))
}
sym::unchecked_shl | sym::unchecked_shr => (2, 0, vec![param(0), param(1)], param(0)),
sym::rotate_left | sym::rotate_right => (1, 0, vec![param(0), param(0)], param(0)),
sym::rotate_left | sym::rotate_right => (1, 0, vec![param(0), tcx.types.u32], param(0)),
sym::unchecked_add | sym::unchecked_sub | sym::unchecked_mul => {
(1, 0, vec![param(0), param(0)], param(0))
}
Expand Down
47 changes: 47 additions & 0 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,13 @@ extern "rust-intrinsic" {
/// The stabilized versions of this intrinsic are available on the integer
/// primitives via the `count_ones` method. For example,
/// [`u32::count_ones`]
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_ctpop", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn ctpop<T: Copy>(x: T) -> u32;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_ctpop", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
Expand Down Expand Up @@ -2028,6 +2035,13 @@ extern "rust-intrinsic" {
/// let num_leading = ctlz(x);
/// assert_eq!(num_leading, 16);
/// ```
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_ctlz", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn ctlz<T: Copy>(x: T) -> u32;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_ctlz", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
Expand All @@ -2050,6 +2064,12 @@ extern "rust-intrinsic" {
/// let num_leading = unsafe { ctlz_nonzero(x) };
/// assert_eq!(num_leading, 3);
/// ```
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "constctlz", since = "1.50.0")]
#[rustc_nounwind]
pub fn ctlz_nonzero<T: Copy>(x: T) -> u32;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "constctlz", since = "1.50.0")]
#[rustc_nounwind]
pub fn ctlz_nonzero<T: Copy>(x: T) -> T;
Expand Down Expand Up @@ -2090,6 +2110,13 @@ extern "rust-intrinsic" {
/// let num_trailing = cttz(x);
/// assert_eq!(num_trailing, 16);
/// ```
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_cttz", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn cttz<T: Copy>(x: T) -> u32;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_cttz", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
Expand All @@ -2112,6 +2139,12 @@ extern "rust-intrinsic" {
/// let num_trailing = unsafe { cttz_nonzero(x) };
/// assert_eq!(num_trailing, 3);
/// ```
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_cttz_nonzero", since = "1.53.0")]
#[rustc_nounwind]
pub fn cttz_nonzero<T: Copy>(x: T) -> u32;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_cttz_nonzero", since = "1.53.0")]
#[rustc_nounwind]
pub fn cttz_nonzero<T: Copy>(x: T) -> T;
Expand Down Expand Up @@ -2288,6 +2321,13 @@ extern "rust-intrinsic" {
/// The stabilized versions of this intrinsic are available on the integer
/// primitives via the `rotate_left` method. For example,
/// [`u32::rotate_left`]
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_int_rotate", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn rotate_left<T: Copy>(x: T, shift: u32) -> T;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_int_rotate", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
Expand All @@ -2303,6 +2343,13 @@ extern "rust-intrinsic" {
/// The stabilized versions of this intrinsic are available on the integer
/// primitives via the `rotate_right` method. For example,
/// [`u32::rotate_right`]
#[cfg(not(bootstrap))]
#[rustc_const_stable(feature = "const_int_rotate", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn rotate_right<T: Copy>(x: T, shift: u32) -> T;

#[cfg(bootstrap)]
#[rustc_const_stable(feature = "const_int_rotate", since = "1.40.0")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
Expand Down
14 changes: 12 additions & 2 deletions library/core/src/num/nonzero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,12 @@ macro_rules! nonzero_integer {
#[inline]
pub const fn leading_zeros(self) -> u32 {
// SAFETY: since `self` cannot be zero, it is safe to call `ctlz_nonzero`.
unsafe { intrinsics::ctlz_nonzero(self.get() as $UnsignedPrimitive) as u32 }
unsafe {
#[cfg(not(bootstrap))]
return intrinsics::ctlz_nonzero(self.get() as $UnsignedPrimitive);
#[cfg(bootstrap)]
return intrinsics::ctlz_nonzero(self.get() as $UnsignedPrimitive) as u32;
}
}

/// Returns the number of trailing zeros in the binary representation
Expand All @@ -552,7 +557,12 @@ macro_rules! nonzero_integer {
#[inline]
pub const fn trailing_zeros(self) -> u32 {
// SAFETY: since `self` cannot be zero, it is safe to call `cttz_nonzero`.
unsafe { intrinsics::cttz_nonzero(self.get() as $UnsignedPrimitive) as u32 }
unsafe {
#[cfg(not(bootstrap))]
return intrinsics::cttz_nonzero(self.get() as $UnsignedPrimitive);
#[cfg(bootstrap)]
return intrinsics::cttz_nonzero(self.get() as $UnsignedPrimitive) as u32;
}
}

/// Returns the number of ones in the binary representation of `self`.
Expand Down
Loading
Loading