Add intrinsics for bigint helper methods #131566
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,6 +178,34 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
| )?; | ||
| self.write_scalar(val, dest)?; | ||
| } | ||
| sym::add_with_carry | sym::sub_with_carry => { | ||
| let l = self.read_immediate(&args[0])?; | ||
| let r = self.read_immediate(&args[1])?; | ||
| let c = self.read_immediate(&args[2])?; | ||
| let (val, overflowed) = self.carrying_arith( | ||
| if intrinsic_name == sym::add_with_carry { BinOp::Add } else { BinOp::Sub }, | ||
| &l, | ||
| &r, | ||
| &c, | ||
| )?; | ||
| self.write_scalar_pair(val, overflowed, dest)?; | ||
| } | ||
| sym::mul_double | sym::mul_double_add | sym::mul_double_add2 => { | ||
| let l = self.read_immediate(&args[0])?; | ||
| let r = self.read_immediate(&args[1])?; | ||
| let c1 = if intrinsic_name != sym::mul_double { | ||
| Some(self.read_immediate(&args[2])?) | ||
| } else { | ||
| None | ||
| }; | ||
| let c2 = if intrinsic_name == sym::mul_double_add2 { | ||
| Some(self.read_immediate(&args[3])?) | ||
| } else { | ||
| None | ||
| }; | ||
| let (lo, hi) = self.mul_double_add2(&l, &r, c1.as_ref(), c2.as_ref())?; | ||
| self.write_scalar_pair(lo, hi, dest)?; | ||
| } | ||
| sym::discriminant_value => { | ||
| let place = self.deref_pointer(&args[0])?; | ||
| let variant = self.read_discriminant(&place)?; | ||
|
|
@@ -573,6 +601,106 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
| }) | ||
| } | ||
|
|
||
| pub fn carrying_arith( | ||
| &self, | ||
| mir_op: BinOp, | ||
| l: &ImmTy<'tcx, M::Provenance>, | ||
| r: &ImmTy<'tcx, M::Provenance>, | ||
| c: &ImmTy<'tcx, M::Provenance>, | ||
| ) -> InterpResult<'tcx, (Scalar<M::Provenance>, Scalar<M::Provenance>)> { | ||
| assert_eq!(l.layout.ty, r.layout.ty); | ||
| assert_matches!(l.layout.ty.kind(), ty::Int(..) | ty::Uint(..)); | ||
| assert_matches!(c.layout.ty.kind(), ty::Bool); | ||
| assert_matches!(mir_op, BinOp::Add | BinOp::Sub); | ||
|
|
||
| let mir_op = mir_op.wrapping_to_overflowing().unwrap(); | ||
|
|
||
| let (val, overflowed1) = self.binary_op(mir_op, l, r)?.to_scalar_pair(); | ||
|
|
||
| let val = ImmTy::from_scalar(val, l.layout); | ||
| let c = ImmTy::from_scalar(c.to_scalar(), l.layout); | ||
|
|
||
| let (val, overflowed2) = self.binary_op(mir_op, &val, &c)?.to_scalar_pair(); | ||
|
|
||
| let overflowed1 = overflowed1.to_bool()?; | ||
| let overflowed2 = overflowed2.to_bool()?; | ||
|
|
||
| let overflowed = Scalar::from_bool(if l.layout.abi.is_signed() { | ||
| overflowed1 != overflowed2 | ||
| } else { | ||
| overflowed1 | overflowed2 | ||
| }); | ||
|
Comment on lines
+628
to
+632
There was a problem hiding this comment. Please add a comment explaining the logic here There was a problem hiding this comment. I definitely should just update the intrinsic docs to clarify the behaviour here, although the standard library has a bit of a weird relationship with how it documents intrinsics. Most of them rely on having stabilised versions that they can just point to, since those will have the proper docs. The documentation here is split between Not sure what the best solution for documentation would be here; open to ideas. I could just link those docs here, for now. There was a problem hiding this comment. I just don't understand why the signed thing overflowed if exactly one of the sub-operations overflowed. Like I could probably think about it for a minute and figure it out, but there should really be comments explaining that. Your answer confused me even more, in which sense is the result different for signed vs unsigned? That should also be documented... |
||
|
|
||
| interp_ok((val, overflowed)) | ||
| } | ||
|
|
||
| pub fn mul_double_add2( | ||
| &self, | ||
| l: &ImmTy<'tcx, M::Provenance>, | ||
| r: &ImmTy<'tcx, M::Provenance>, | ||
| c1: Option<&ImmTy<'tcx, M::Provenance>>, | ||
| c2: Option<&ImmTy<'tcx, M::Provenance>>, | ||
| ) -> InterpResult<'tcx, (Scalar<M::Provenance>, Scalar<M::Provenance>)> { | ||
| assert_eq!(l.layout.ty, r.layout.ty); | ||
| assert_matches!(l.layout.ty.kind(), ty::Int(..) | ty::Uint(..)); | ||
|
|
||
| let is_signed = l.layout.abi.is_signed(); | ||
| let size = l.layout.size; | ||
| let bits = size.bits(); | ||
| let l = l.to_scalar_int()?; | ||
| let r = r.to_scalar_int()?; | ||
|
|
||
| interp_ok(if is_signed { | ||
| let l = l.to_int(size); | ||
| let r = r.to_int(size); | ||
| let c1 = c1.map_or(interp_ok(0), |c1| interp_ok(c1.to_scalar_int()?.to_int(size)))?; | ||
| let c2 = c2.map_or(interp_ok(0), |c2| interp_ok(c2.to_scalar_int()?.to_int(size)))?; | ||
| if bits == 128 { | ||
| #[cfg(bootstrap)] | ||
| { | ||
| let _ = (l, r, c1, c2); | ||
| unimplemented!() | ||
| } | ||
| #[cfg(not(bootstrap))] | ||
| { | ||
| let (lo, hi) = l.carrying2_mul(r, c1, c2); | ||
| let lo = Scalar::from_uint(lo, size); | ||
| let hi = Scalar::from_int(hi, size); | ||
| (lo, hi) | ||
| } | ||
| } else { | ||
| let prod = l * r + c1 + c2; | ||
| let lo = Scalar::from_int(prod, size); | ||
| let hi = Scalar::from_int(prod >> size.bits(), size); | ||
| (lo, hi) | ||
| } | ||
| } else { | ||
| let l = l.to_uint(size); | ||
| let r = r.to_uint(size); | ||
| let c1 = c1.map_or(interp_ok(0), |c1| interp_ok(c1.to_scalar_int()?.to_uint(size)))?; | ||
| let c2 = c2.map_or(interp_ok(0), |c2| interp_ok(c2.to_scalar_int()?.to_uint(size)))?; | ||
| if bits == 128 { | ||
| #[cfg(bootstrap)] | ||
| { | ||
| let _ = (l, r, c1, c2); | ||
| unimplemented!() | ||
| } | ||
| #[cfg(not(bootstrap))] | ||
| { | ||
| let (lo, hi) = l.carrying2_mul(r, c1, c2); | ||
|
There was a problem hiding this comment. This is unfortunate... for basic arithmetic like this, it'd be better if we had our own implementation of them rather than having to use the host operation. How hard would that be? There was a problem hiding this comment. So, my thought process here is that the ideal solution is to replace the current hard-coded However, without that, my thought process was that I could either manually code a version of I'll defer to whatever you think is the better option, but that at least explains my reasoning. There was a problem hiding this comment. I don't know how hard this is to implement directly, is there some code somewhere that would give me an idea? It's also not great to have a completely different codepath for u128 and the rest, that makes proper testing more tricky. There was a problem hiding this comment. So, just sharing a few of the implementations mentioned on the tracking issue:
Note that also, regardless of what we do, the result of double-wide multiplication of 128-bit integers is going to be two 128-bit integers, and it's only going to be the case of 128-bit integers where we need to scoop out the extra data from the more-significant 128 bits. So, effectively, even if I had the same path for all integers using the 128-bit double-wide mul, we'd still be special-casing 128 bits by only looking at the higher-order word in the 128-bit case. There was a problem hiding this comment.
FWIW I'd love that for all our arithmetic.^^ It's probably too slow though. And using it only sometimes seems odd. There was a problem hiding this comment.
So, my thought process here is that we typically want to be independent from possible bugs in the standard library, and provide our own reference implementation. But, we haven't done that in numeric_intrinsic, so it'd be odd to use a higher standard here. So fair, please stick with the current implementation, just with more comments. |
||
| let lo = Scalar::from_uint(lo, size); | ||
| let hi = Scalar::from_uint(hi, size); | ||
| (lo, hi) | ||
| } | ||
| } else { | ||
| let prod = l * r + c1 + c2; | ||
|
There was a problem hiding this comment. Please add a comment explaining why this does not cause overflow. Also please use There was a problem hiding this comment. Will convert to strict, although I thought that the lack of overflow was evident by the fact that the 128-bit case was covered separately-- all other integers would be maximum 64 bits, where this operation cannot overflow. There was a problem hiding this comment. You are multiplying two 64bit numbers here, which results in a 128bit number. Then you add more stuff. There was a problem hiding this comment. It turns out that But yes, I'll add comments. |
||
| let lo = Scalar::from_uint(prod, size); | ||
| let hi = Scalar::from_uint(prod >> size.bits(), size); | ||
| (lo, hi) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /// Offsets a pointer by some multiple of its type, returning an error if the pointer leaves its | ||
| /// allocation. | ||
| pub fn ptr_offset_inbounds( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a doc comment to both new methods