Repository navigation
Add SIMD algebraic intrinsics - #164952
calebzulawski wants to merge 10 commits into
Conversation
|
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the platform-builtins intrinsics. Make sure the cc @antoyo, @GuillaumeGomez, @bjorn3, @programmerjake Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter |
|
r? @davidtwco rustbot has assigned @davidtwco. Use Why was this reviewer chosen?The reviewer was selected based on:
|
e92596e to
9d8b178
Compare
| sym::simd_reduce_add_algebraic | sym::simd_reduce_mul_algebraic | ||
| ); | ||
| let mut res = init; | ||
| for i in 0..op_len { |
There was a problem hiding this comment.
the unordered ops should probably also pick a random order to reduce in, but imo that doesn't need to be part of this PR.
There was a problem hiding this comment.
In const-eval the order must be deterministic so we can't randomize.
However, Miri needs to randomize them. With this PR Miri will start running them, without proper randomization. That's a bug introduced by this PR. So I disagree, if we have Miri support here then it must be correct.
| sym::simd_add_algebraic => Op::MirOp(BinOp::Add), | ||
| sym::simd_sub_algebraic => Op::MirOp(BinOp::Sub), | ||
| sym::simd_mul_algebraic => Op::MirOp(BinOp::Mul), | ||
| sym::simd_div_algebraic => Op::MirOp(BinOp::Div), | ||
| sym::simd_rem_algebraic => Op::MirOp(BinOp::Rem), |
There was a problem hiding this comment.
Seems cleaner to represent this as a MirOpFloatAlgebraic or so to avoid having to match twice.
| if val.layout.ty.is_floating_point() && nondet { | ||
| val = M::apply_float_nondet(self, val)?; | ||
| } |
There was a problem hiding this comment.
To ensure that this really makes things non-deterministic, please add a test like this in the tests/pass/intrinsics/portable-simd.rs file.
|
cc @rust-lang/miri |
|
cc @bjorn3 |
3125bca to
6796d73
Compare
| }; | ||
|
|
||
| let mut res = init; | ||
| for i in M::nondet_index_iterator(self, op_len) { |
There was a problem hiding this comment.
maybe add a comment noting that this doesn't fully randomize the reduce order, since it only randomizes the order of the inputs then does an ordered reduce -- that will never give you an evaluation order like (a + b) + (c + d).
There was a problem hiding this comment.
an example implementation that does cover all possible evaluation orders: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c2f778ccd6b16d881b5afac8200ffaf7
pub fn random_reduce<T>(mut inputs: Vec<T>, mut reduce: impl FnMut(T, T) -> T) -> Option<T> {
while inputs.len() >= 2 {
let lhs_index = rand::random_range(0..inputs.len());
let lhs = inputs.swap_remove(lhs_index);
let rhs_index = rand::random_range(0..inputs.len());
let rhs = inputs.swap_remove(rhs_index);
inputs.push(reduce(lhs, rhs));
}
inputs.pop()
}There was a problem hiding this comment.
I'm curious if this is actually necessary, miri's float nondeterminism doesn't generate every possible distribution of nondeterminism
There was a problem hiding this comment.
maybe not, but in the case of catastrophic cancellation, you can get wildly different results for the different evaluation orders so imo it would be useful for miri to be able to give you those.
This comment has been minimized.
This comment has been minimized.
4db7858 to
3067057
Compare
This comment has been minimized.
This comment has been minimized.
…educe order, and add non-determinism miri test for SIMD
… regular operations
Co-authored-by: Ralf Jung <post@ralfj.de>
3067057 to
95f5629
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I think #164924 introduced a minor break to cg_gcc vector frem, I repaired that here |
| #[inline(always)] | ||
| fn nondet_index_iterator(ecx: &mut InterpCx<'tcx, Self>, len: u64) -> impl Iterator<Item = u64> + use<'tcx> { | ||
| let mut indices = (0..len).collect::<Vec<u64>>(); | ||
| if ecx.machine.float_nondet { |
There was a problem hiding this comment.
Since the implementation is float-specific, let's call this unordered_float_reduction_iter or something like that.
|
|
||
| let (op, op_len) = self.project_to_simd(&args[0])?; | ||
|
|
||
| let scalar = |f: Double| { |
There was a problem hiding this comment.
If you are creating the initial value via casts anyway, might as well use int_to_int_or_float and cast from an integer, that should be a bit simpler.
| for i in M::nondet_index_iterator(self, op_len) { | ||
| let op = self.read_immediate(&self.project_index(&op, i)?)?; | ||
| res = self.binary_op(mir_op, &res, &op)?; | ||
| if dest.layout.ty.is_floating_point() { |
There was a problem hiding this comment.
This should only be called on float types anyway, right?
|
|
||
| let (op, op_len) = self.project_to_simd(&args[0])?; | ||
|
|
||
| let scalar = |f: Double| { |
There was a problem hiding this comment.
| let scalar = |f: Double| { | |
| // Compute starting value and reduction MIR op. | |
| let from_int = |i: i32| { |
View all comments
Added algebraic intrinsics for SIMD. First half of rust-lang/portable-simd#539