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.

Sign up for GitHub

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

Use a C-safe return type for __rust_[ui]128_* overflowing intrinsics #134338

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Use a C-safe return type for __rust_[ui]128_* overflowing intrinsics #134338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a911da1
Use a C-safe return type for `__rust_[ui]128_*` overflowing intrinsics
tgross35 Dec 15, 2024
f6a2db8
Update compiler-builtins to 0.1.143
tgross35 Jan 15, 2025
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
4 changes: 2 additions & 2 deletions ...stc_codegen_cranelift/patches/0029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ index 7165c3e48af..968552ad435 100644

[dependencies]
core = { path = "../core" }
-compiler_builtins = { version = "=0.1.141", features = ['rustc-dep-of-std'] }
+compiler_builtins = { version = "=0.1.141", features = ['rustc-dep-of-std', 'no-f16-f128'] }
-compiler_builtins = { version = "=0.1.143", features = ['rustc-dep-of-std'] }
+compiler_builtins = { version = "=0.1.143", features = ['rustc-dep-of-std', 'no-f16-f128'] }

[dev-dependencies]
rand = { version = "0.8.5", default-features = false, features = ["alloc"] }
Expand Down
18 changes: 10 additions & 8 deletions compiler/rustc_codegen_cranelift/src/codegen_i128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,22 @@ pub(crate) fn maybe_codegen_mul_checked<'tcx>(
}

let is_signed = type_sign(lhs.layout().ty);

let out_ty = Ty::new_tup(fx.tcx, &[lhs.layout().ty, fx.tcx.types.bool]);
let out_place = CPlace::new_stack_slot(fx, fx.layout_of(out_ty));
let oflow_out_place = CPlace::new_stack_slot(fx, fx.layout_of(fx.tcx.types.i32));
let param_types = vec![
AbiParam::special(fx.pointer_type, ArgumentPurpose::StructReturn),
AbiParam::new(types::I128),
AbiParam::new(types::I128),
AbiParam::special(fx.pointer_type, ArgumentPurpose::Normal),
];
let args = [out_place.to_ptr().get_addr(fx), lhs.load_scalar(fx), rhs.load_scalar(fx)];
fx.lib_call(
let args = [lhs.load_scalar(fx), rhs.load_scalar(fx), oflow_out_place.to_ptr().get_addr(fx)];
let ret = fx.lib_call(
if is_signed { "__rust_i128_mulo" } else { "__rust_u128_mulo" },
param_types,
vec![],
vec![AbiParam::new(types::I128)],
&args,
);
Some(out_place.to_cvalue(fx))
let mul = ret[0];
let oflow = oflow_out_place.to_cvalue(fx).load_scalar(fx);
let oflow = clif_intcast(fx, oflow, types::I8, false);
let layout = fx.layout_of(Ty::new_tup(fx.tcx, &[lhs.layout().ty, fx.tcx.types.bool]));
Some(CValue::by_val_pair(mul, oflow, layout))
}
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/compiler_builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ builtin_functions! {
fn __divti3(n: i128, d: i128) -> i128;
fn __umodti3(n: u128, d: u128) -> u128;
fn __modti3(n: i128, d: i128) -> i128;
fn __rust_u128_mulo(a: u128, b: u128) -> (u128, bool);
fn __rust_u128_mulo(a: u128, b: u128, oflow: &mut i32) -> u128;

// floats
fn __floattisf(i: i128) -> f32;
Expand Down
123 changes: 60 additions & 63 deletions compiler/rustc_codegen_gcc/src/int.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,36 +322,26 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> {
},
}
} else {
match new_kind {
Int(I128) | Uint(U128) => {
let func_name = match oop {
OverflowOp::Add => match new_kind {
Int(I128) => "__rust_i128_addo",
Uint(U128) => "__rust_u128_addo",
_ => unreachable!(),
},
OverflowOp::Sub => match new_kind {
Int(I128) => "__rust_i128_subo",
Uint(U128) => "__rust_u128_subo",
_ => unreachable!(),
},
OverflowOp::Mul => match new_kind {
Int(I128) => "__rust_i128_mulo", // TODO(antoyo): use __muloti4d instead?
Uint(U128) => "__rust_u128_mulo",
_ => unreachable!(),
},
};
return self.operation_with_overflow(func_name, lhs, rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see, this change doesn't look right as this will fall to calling self.overflow_call() below which expects that the overflow flag is returned from the function call (like these GCC intrinsics) whereas your change seems to return the integer result and not the overflow flag.

My guess is that you'll need to keep this special handling and change the method operation_with_overflow accordingly as these intrinsics are also used in src/intrinsic/mod.rs.

AFAIK, the only related tests that run in this CI are those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

AFAIK, the only related tests that run in this CI are those lines.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

Indeed, it looks like there might be a bug in here.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it looks like there might be a bug in here.

I changed this so mulo and the __rust functions should now be taking the same codepath, still have to update the rest.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

I am just wondering whether I should add another test anywhere to verify the new behavior, especially if __mulosi4 may have been called incorrectly before. And if so, what format/location this should be (for LLVM I'd probably add a codegen test, but I'm not sure what the equivalent would be here).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting that there would be UI tests for these, so that every backend could benefit from them.
Do they have to be codegen tests?

There are integer tests here for the GCC codegen, but they are not ran in the Rust CI, as far as I know, so if it's too much of a pain, we can open an issue to remind me to add tests for this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at https://rust.godbolt.org/z/eWfPK6rGW it seems like GCC lowers these operations to assembly (LLVM as well), which possibly explains why the test was not failing before even if the __mulosi4 call had a bug. I am not very familiar with this backend but I take it GCC intercepts the call to __builtin_sub_overflow and adjusts for the correct size?

If it doesn't emit this call then I am not sure what kind of test would work here, but can add something if you have any ideas. I updated the code and it should be ready for review, it builds but have not been able to run tests.

I was expecting that there would be UI tests for these, so that every backend could benefit from them.

Agreed that we should have something end-to-end here, possibly reusing the test generators from compiler-builtins/libm. I think the status quo assumption is that we don't need to test the lowering too heavily since that is the backend's responsibility, but it would still be good to have something systematic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very familiar with this backend but I take it GCC intercepts the call to __builtin_sub_overflow and adjusts for the correct size?

Yes. The doc mentions:

The first built-in function allows arbitrary integral types for operands and the result type must be pointer to some integral type other than enumerated or boolean type, the rest of the built-in functions have explicit integer types.

I reviewed your code: it looks good, thanks!

This test does call overflowing_mul, so I'm not sure why it wasn't tested there.
Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does call overflowing_mul, so I'm not sure why it wasn't tested there. Any ideas?

In order to hit the branch where __mulosi/__mulodi gets emitted, the self.is_native_int_type(lhs.get_type()) condition has to fail for i32 or i64 - I'm guessing there probably aren't many platforms where that would happen. The calls to __rust_i128_* were correct before but it looks like is_native_int_type has to be returning true for i128 too, at least on x86, since that gets asm lowering https://rust.godbolt.org/z/jPfMK5vhv. I take it maybe the gcc-13-without-int128 CI jobs in the cg_gcc repo would cover this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that CI job uses a GCC where 128-bit integers are disabled to test this indeed.

Right, that must be why it wasn't tested. But I think i64 is supported on all platforms, so I'll open an issue to check that.

}
_ => match oop {
OverflowOp::Mul => match new_kind {
Int(I32) => "__mulosi4",
Int(I64) => "__mulodi4",
_ => unreachable!(),
},
_ => unimplemented!("overflow operation for {:?}", new_kind),
let (func_name, width) = match oop {
OverflowOp::Add => match new_kind {
Int(I128) => ("__rust_i128_addo", 128),
Uint(U128) => ("__rust_u128_addo", 128),
_ => unreachable!(),
},
}
OverflowOp::Sub => match new_kind {
Int(I128) => ("__rust_i128_subo", 128),
Uint(U128) => ("__rust_u128_subo", 128),
_ => unreachable!(),
},
OverflowOp::Mul => match new_kind {
Int(I32) => ("__mulosi4", 32),
Int(I64) => ("__mulodi4", 64),
Int(I128) => ("__rust_i128_mulo", 128), // TODO(antoyo): use __muloti4d instead?
Uint(U128) => ("__rust_u128_mulo", 128),
_ => unreachable!(),
},
};
return self.operation_with_overflow(func_name, lhs, rhs, width);
};

let intrinsic = self.context.get_builtin_function(name);
Expand All @@ -364,80 +354,87 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> {
(res.dereference(self.location).to_rvalue(), overflow)
}

/// Non-`__builtin_*` overflow operations with a `fn(T, T, &mut i32) -> T` signature.
pub fn operation_with_overflow(
&self,
func_name: &str,
lhs: RValue<'gcc>,
rhs: RValue<'gcc>,
width: u64,
) -> (RValue<'gcc>, RValue<'gcc>) {
let a_type = lhs.get_type();
let b_type = rhs.get_type();
debug_assert!(a_type.dyncast_array().is_some());
debug_assert!(b_type.dyncast_array().is_some());
let overflow_type = self.i32_type;
let overflow_param_type = overflow_type.make_pointer();
let res_type = a_type;

let overflow_value =
self.current_func().new_local(self.location, overflow_type, "overflow");
let overflow_addr = overflow_value.get_address(self.location);

let param_a = self.context.new_parameter(self.location, a_type, "a");
let param_b = self.context.new_parameter(self.location, b_type, "b");
let result_field = self.context.new_field(self.location, a_type, "result");
let overflow_field = self.context.new_field(self.location, self.bool_type, "overflow");

let ret_ty = Ty::new_tup(self.tcx, &[self.tcx.types.i128, self.tcx.types.bool]);
let param_overflow =
self.context.new_parameter(self.location, overflow_param_type, "overflow");

let a_elem_type = a_type.dyncast_array().expect("non-array a value");
debug_assert!(a_elem_type.is_integral());
let res_ty = match width {
32 => self.tcx.types.i32,
64 => self.tcx.types.i64,
128 => self.tcx.types.i128,
_ => unreachable!("unexpected integer size"),
};
Comment on lines +384 to +389
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason to do that instead of reusing a_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe a_type is the GCC type and I need the rustc type to calculate the layout. I could not find a way to convert in this direction - is there one? Or a way to get a layout directly from the GCC type (feel like I might be missing something here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry I forgot that you now have multiple types to handle here and not only 128-bit integers.
It's good as it is.

let layout = self
.tcx
.layout_of(ty::TypingEnv::fully_monomorphized().as_query_input(ret_ty))
.layout_of(ty::TypingEnv::fully_monomorphized().as_query_input(res_ty))
.unwrap();

let arg_abi = ArgAbi { layout, mode: PassMode::Direct(ArgAttributes::new()) };
let mut fn_abi = FnAbi {
args: vec![arg_abi.clone(), arg_abi.clone()].into_boxed_slice(),
args: vec![arg_abi.clone(), arg_abi.clone(), arg_abi.clone()].into_boxed_slice(),
ret: arg_abi,
c_variadic: false,
fixed_count: 2,
fixed_count: 3,
conv: Conv::C,
can_unwind: false,
};
fn_abi.adjust_for_foreign_abi(self.cx, spec::abi::Abi::C { unwind: false }).unwrap();

let indirect = matches!(fn_abi.ret.mode, PassMode::Indirect { .. });

let return_type = self
.context
.new_struct_type(self.location, "result_overflow", &[result_field, overflow_field]);
let result = if indirect {
let return_value =
self.current_func().new_local(self.location, return_type.as_type(), "return_value");
let return_param_type = return_type.as_type().make_pointer();
let return_param =
self.context.new_parameter(self.location, return_param_type, "return_value");
let ret_indirect = matches!(fn_abi.ret.mode, PassMode::Indirect { .. });

let result = if ret_indirect {
let res_value = self.current_func().new_local(self.location, res_type, "result_value");
let res_addr = res_value.get_address(self.location);
let res_param_type = res_type.make_pointer();
let param_res = self.context.new_parameter(self.location, res_param_type, "result");

let func = self.context.new_function(
self.location,
FunctionType::Extern,
self.type_void(),
&[return_param, param_a, param_b],
&[param_res, param_a, param_b, param_overflow],
func_name,
false,
);
self.llbb().add_eval(
self.location,
self.context.new_call(self.location, func, &[
return_value.get_address(self.location),
lhs,
rhs,
]),
);
return_value.to_rvalue()
let _void =
self.context.new_call(self.location, func, &[res_addr, lhs, rhs, overflow_addr]);
res_value.to_rvalue()
} else {
let func = self.context.new_function(
self.location,
FunctionType::Extern,
return_type.as_type(),
&[param_a, param_b],
res_type,
&[param_a, param_b, param_overflow],
func_name,
false,
);
self.context.new_call(self.location, func, &[lhs, rhs])
self.context.new_call(self.location, func, &[lhs, rhs, overflow_addr])
};
let overflow = result.access_field(self.location, overflow_field);
let int_result = result.access_field(self.location, result_field);
(int_result, overflow)

(result, self.context.new_cast(self.location, overflow_value, self.bool_type).to_rvalue())
}

pub fn gcc_icmp(
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_codegen_gcc/src/intrinsic/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,7 +1001,8 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> {
128 => "__rust_i128_addo",
_ => unreachable!(),
};
let (int_result, overflow) = self.operation_with_overflow(func_name, lhs, rhs);
let (int_result, overflow) =
self.operation_with_overflow(func_name, lhs, rhs, width);
self.llbb().add_assignment(self.location, res, int_result);
overflow
};
Expand Down Expand Up @@ -1071,7 +1072,8 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> {
128 => "__rust_i128_subo",
_ => unreachable!(),
};
let (int_result, overflow) = self.operation_with_overflow(func_name, lhs, rhs);
let (int_result, overflow) =
self.operation_with_overflow(func_name, lhs, rhs, width);
self.llbb().add_assignment(self.location, res, int_result);
overflow
};
Expand Down
4 changes: 2 additions & 2 deletions library/Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ dependencies = [

[[package]]
name = "compiler_builtins"
version = "0.1.141"
version = "0.1.143"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b5e7a0206befe4e574e37d6d7a0fe82e88fdf54bedb0608f239cb11b7a6aa6be"
checksum = "c85ba2077e3eab3dd81be4ece6b7fb2ad0887c1fb813e9a45400baf75c6c7c29"
dependencies = [
"cc",
"rustc-std-workspace-core",
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ edition = "2021"

[dependencies]
core = { path = "../core" }
compiler_builtins = { version = "=0.1.141", features = ['rustc-dep-of-std'] }
compiler_builtins = { version = "=0.1.143", features = ['rustc-dep-of-std'] }

[dev-dependencies]
rand = { version = "0.8.5", default-features = false, features = ["alloc"] }
Expand Down
2 changes: 1 addition & 1 deletion library/std/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ cfg-if = { version = "1.0", features = ['rustc-dep-of-std'] }
panic_unwind = { path = "../panic_unwind", optional = true }
panic_abort = { path = "../panic_abort" }
core = { path = "../core", public = true }
compiler_builtins = { version = "=0.1.141" }
compiler_builtins = { version = "=0.1.143" }
unwind = { path = "../unwind" }
hashbrown = { version = "0.15", default-features = false, features = [
'rustc-dep-of-std',
Expand Down
Loading