From 005a17441b22c523786434a124a5f700242f9057 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 10 Dec 2024 17:04:53 -0800 Subject: [PATCH] pulley: Fill out remaining 32/64-bit integer operations This required some extra plumbing to shepherd the precise reason why signed division trapped to Wasmtime which is done through an extra `TrapKind` side channel now added. This then additionally fixes the signed remainder interpreter function to return 0 on `MIN % -1` which is different from what Rust specifies (which is to return `None` or panic). cc #9783 --- .../src/isa/pulley_shared/inst/emit.rs | 12 -- .../codegen/src/isa/pulley_shared/lower.isle | 15 +++ crates/wasmtime/src/runtime/vm/interpreter.rs | 65 +++++++---- crates/wast-util/src/lib.rs | 4 - pulley/src/interp.rs | 109 +++++++++++++++--- pulley/src/lib.rs | 15 +++ 6 files changed, 166 insertions(+), 54 deletions(-) diff --git a/cranelift/codegen/src/isa/pulley_shared/inst/emit.rs b/cranelift/codegen/src/isa/pulley_shared/inst/emit.rs index b03daa80fc8f..d209e6530ebc 100644 --- a/cranelift/codegen/src/isa/pulley_shared/inst/emit.rs +++ b/cranelift/codegen/src/isa/pulley_shared/inst/emit.rs @@ -490,18 +490,6 @@ fn pulley_emit

( RawInst::PushFrame | RawInst::StackAlloc32 { .. } => { sink.add_trap(ir::TrapCode::STACK_OVERFLOW); } - RawInst::XDiv32U { .. } - | RawInst::XDiv64U { .. } - | RawInst::XRem32U { .. } - | RawInst::XRem64U { .. } => { - sink.add_trap(ir::TrapCode::INTEGER_DIVISION_BY_ZERO); - } - RawInst::XDiv32S { .. } - | RawInst::XDiv64S { .. } - | RawInst::XRem32S { .. } - | RawInst::XRem64S { .. } => { - sink.add_trap(ir::TrapCode::INTEGER_OVERFLOW); - } _ => {} } super::generated::emit(raw, sink) diff --git a/cranelift/codegen/src/isa/pulley_shared/lower.isle b/cranelift/codegen/src/isa/pulley_shared/lower.isle index f283d0917426..e4eecca589bc 100644 --- a/cranelift/codegen/src/isa/pulley_shared/lower.isle +++ b/cranelift/codegen/src/isa/pulley_shared/lower.isle @@ -251,6 +251,21 @@ (rule (lower (has_type $I32 (clz a))) (pulley_xclz32 a)) (rule (lower (has_type $I64 (clz a))) (pulley_xclz64 a)) +;;;; Rules for `popcnt` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(rule (lower (has_type $I32 (popcnt a))) (pulley_xpopcnt32 a)) +(rule (lower (has_type $I64 (popcnt a))) (pulley_xpopcnt64 a)) + +;;;; Rules for `rotl` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(rule (lower (has_type $I32 (rotl a b))) (pulley_xrotl32 a b)) +(rule (lower (has_type $I64 (rotl a b))) (pulley_xrotl64 a b)) + +;;;; Rules for `rotr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(rule (lower (has_type $I32 (rotr a b))) (pulley_xrotr32 a b)) +(rule (lower (has_type $I64 (rotr a b))) (pulley_xrotr64 a b)) + ;;;; Rules for `icmp` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (rule 1 (lower (icmp cc a b @ (value_type $I64))) diff --git a/crates/wasmtime/src/runtime/vm/interpreter.rs b/crates/wasmtime/src/runtime/vm/interpreter.rs index 6d0349b9d504..2fa2e6b07b24 100644 --- a/crates/wasmtime/src/runtime/vm/interpreter.rs +++ b/crates/wasmtime/src/runtime/vm/interpreter.rs @@ -3,9 +3,9 @@ use crate::runtime::vm::vmcontext::VMArrayCallNative; use crate::runtime::vm::{tls, TrapRegisters, TrapTest, VMContext, VMOpaqueContext}; use crate::ValRaw; use core::ptr::NonNull; -use pulley_interpreter::interp::{DoneReason, RegType, Val, Vm, XRegVal}; +use pulley_interpreter::interp::{DoneReason, RegType, TrapKind, Val, Vm, XRegVal}; use pulley_interpreter::{Reg, XReg}; -use wasmtime_environ::{BuiltinFunctionIndex, HostCall}; +use wasmtime_environ::{BuiltinFunctionIndex, HostCall, Trap}; /// Interpreter state stored within a `Store`. #[repr(transparent)] @@ -109,8 +109,8 @@ impl InterpreterRef<'_> { } } // If the VM trapped then process that here and return `false`. - DoneReason::Trap(pc) => { - self.trap(pc, setjmp); + DoneReason::Trap { pc, kind } => { + self.trap(pc, kind, setjmp); break false; } } @@ -125,30 +125,47 @@ impl InterpreterRef<'_> { /// Handles an interpreter trap. This will initialize the trap state stored /// in TLS via the `test_if_trap` helper below by reading the pc/fp of the /// interpreter and seeing if that's a valid opcode to trap at. - fn trap(&mut self, pc: NonNull, setjmp: Setjmp) { - let result = tls::with(|s| { + fn trap(&mut self, pc: NonNull, kind: Option, setjmp: Setjmp) { + let regs = TrapRegisters { + pc: pc.as_ptr() as usize, + fp: self.0[XReg::fp].get_ptr::() as usize, + }; + tls::with(|s| { let s = s.unwrap(); - s.test_if_trap( - TrapRegisters { - pc: pc.as_ptr() as usize, - fp: self.0[XReg::fp].get_ptr::() as usize, - }, - None, - |_| false, - ) - }); + match kind { + Some(kind) => { + let trap = match kind { + TrapKind::IntegerOverflow => Trap::IntegerOverflow, + TrapKind::DivideByZero => Trap::IntegerDivisionByZero, + }; + s.set_jit_trap(regs, None, trap); + } + None => { + let result = s.test_if_trap( + TrapRegisters { + pc: pc.as_ptr() as usize, + fp: self.0[XReg::fp].get_ptr::() as usize, + }, + None, + |_| false, + ); + match result { + // This shouldn't be possible, so this is a fatal error + // if it happens. + TrapTest::NotWasm => { + panic!("pulley trap at {pc:?} without trap code registered") + } - match result { - // This shouldn't be possible, so this is a fatal error if it - // happens. - TrapTest::NotWasm => panic!("pulley trap at {pc:?} without trap code registered"), + // Not possible with our closure above returning `false`. + TrapTest::HandledByEmbedder => unreachable!(), - // Not possible with our closure above returning `false`. - TrapTest::HandledByEmbedder => unreachable!(), + // Trap was handled, yay! We don't use `jmp_buf`. + TrapTest::Trap { jmp_buf: _ } => {} + } + } + } + }); - // Trap was handled, yay! We don't use `jmp_buf`. - TrapTest::Trap { jmp_buf: _ } => {} - } self.longjmp(setjmp); } diff --git a/crates/wast-util/src/lib.rs b/crates/wast-util/src/lib.rs index 22720a4554aa..c4257d0586c8 100644 --- a/crates/wast-util/src/lib.rs +++ b/crates/wast-util/src/lib.rs @@ -407,7 +407,6 @@ impl WastTest { "misc_testsuite/memory-combos.wast", "misc_testsuite/memory64/simd.wast", "misc_testsuite/memory64/threads.wast", - "misc_testsuite/misc_traps.wast", "misc_testsuite/rust_fannkuch.wast", "misc_testsuite/simd/almost-extmul.wast", "misc_testsuite/simd/canonicalize-nan.wast", @@ -443,10 +442,7 @@ impl WastTest { "spec_testsuite/f64_cmp.wast", "spec_testsuite/float_exprs.wast", "spec_testsuite/float_misc.wast", - "spec_testsuite/i32.wast", - "spec_testsuite/i64.wast", "spec_testsuite/imports.wast", - "spec_testsuite/int_exprs.wast", "spec_testsuite/local_get.wast", "spec_testsuite/local_set.wast", "spec_testsuite/local_tee.wast", diff --git a/pulley/src/interp.rs b/pulley/src/interp.rs index ac5d9d8336f5..3e76373a951f 100644 --- a/pulley/src/interp.rs +++ b/pulley/src/interp.rs @@ -82,7 +82,7 @@ impl Vm { match self.call_run(func) { DoneReason::ReturnToHost(()) => DoneReason::ReturnToHost(self.call_end(rets)), - DoneReason::Trap(pc) => DoneReason::Trap(pc), + DoneReason::Trap { pc, kind } => DoneReason::Trap { pc, kind }, DoneReason::CallIndirectHost { id, resume } => { DoneReason::CallIndirectHost { id, resume } } @@ -684,7 +684,12 @@ mod done { /// Reason that the pulley interpreter has ceased execution. pub enum DoneReason { /// A trap happened at this bytecode instruction. - Trap(NonNull), + Trap { + /// Which instruction is raising this trap. + pc: NonNull, + /// The kind of trap being raised, if known. + kind: Option, + }, /// The `call_indirect_host` instruction was executed. CallIndirectHost { /// The payload of `call_indirect_host`. @@ -696,6 +701,13 @@ mod done { ReturnToHost(T), } + /// Stored within `DoneReason::Trap`. + #[expect(missing_docs, reason = "self-describing variants")] + pub enum TrapKind { + DivideByZero, + IntegerOverflow, + } + impl MachineState { pub(super) fn debug_assert_done_reason_none(&mut self) { debug_assert!(self.done_reason.is_none()); @@ -715,8 +727,13 @@ mod done { /// instruction to point to the instruction itself in the trap metadata /// returned from the interpreter. pub fn done_trap(&mut self) -> ControlFlow { + self.done_trap_kind::(None) + } + + /// Same as `done_trap` but with an explicit `TrapKind`. + pub fn done_trap_kind(&mut self, kind: Option) -> ControlFlow { let pc = self.current_pc::(); - self.state.done_reason = Some(DoneReason::Trap(pc)); + self.state.done_reason = Some(DoneReason::Trap { pc, kind }); ControlFlow::Break(Done { _priv: () }) } @@ -738,7 +755,7 @@ mod done { } use done::Done; -pub use done::DoneReason; +pub use done::{DoneReason, TrapKind}; struct Interpreter<'a> { state: &'a mut MachineState, @@ -1583,7 +1600,14 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_i32(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => { + let kind = if b == 0 { + TrapKind::DivideByZero + } else { + TrapKind::IntegerOverflow + }; + self.done_trap_kind::(Some(kind)) + } } } @@ -1595,7 +1619,14 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_i64(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => { + let kind = if b == 0 { + TrapKind::DivideByZero + } else { + TrapKind::IntegerOverflow + }; + self.done_trap_kind::(Some(kind)) + } } } @@ -1607,7 +1638,7 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_u32(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } @@ -1619,31 +1650,41 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_u64(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } fn xrem32_s(&mut self, operands: BinaryOperands) -> ControlFlow { let a = self.state[operands.src1].get_i32(); let b = self.state[operands.src2].get_i32(); - match a.checked_rem(b) { + let result = if a == i32::MIN && b == -1 { + Some(0) + } else { + a.checked_rem(b) + }; + match result { Some(result) => { self.state[operands.dst].set_i32(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } fn xrem64_s(&mut self, operands: BinaryOperands) -> ControlFlow { let a = self.state[operands.src1].get_i64(); let b = self.state[operands.src2].get_i64(); - match a.checked_rem(b) { + let result = if a == i64::MIN && b == -1 { + Some(0) + } else { + a.checked_rem(b) + }; + match result { Some(result) => { self.state[operands.dst].set_i64(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } @@ -1655,7 +1696,7 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_u32(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } @@ -1667,7 +1708,7 @@ impl OpVisitor for Interpreter<'_> { self.state[operands.dst].set_u64(result); ControlFlow::Continue(()) } - None => self.done_trap::(), + None => self.done_trap_kind::(Some(TrapKind::DivideByZero)), } } @@ -1803,6 +1844,46 @@ impl OpVisitor for Interpreter<'_> { ControlFlow::Continue(()) } + fn xpopcnt32(&mut self, dst: XReg, src: XReg) -> ControlFlow { + let a = self.state[src].get_u32(); + self.state[dst].set_u32(a.count_ones()); + ControlFlow::Continue(()) + } + + fn xpopcnt64(&mut self, dst: XReg, src: XReg) -> ControlFlow { + let a = self.state[src].get_u64(); + self.state[dst].set_u64(a.count_ones().into()); + ControlFlow::Continue(()) + } + + fn xrotl32(&mut self, operands: BinaryOperands) -> ControlFlow { + let a = self.state[operands.src1].get_u32(); + let b = self.state[operands.src2].get_u32(); + self.state[operands.dst].set_u32(a.rotate_left(b)); + ControlFlow::Continue(()) + } + + fn xrotl64(&mut self, operands: BinaryOperands) -> ControlFlow { + let a = self.state[operands.src1].get_u64(); + let b = self.state[operands.src2].get_u32(); + self.state[operands.dst].set_u64(a.rotate_left(b)); + ControlFlow::Continue(()) + } + + fn xrotr32(&mut self, operands: BinaryOperands) -> ControlFlow { + let a = self.state[operands.src1].get_u32(); + let b = self.state[operands.src2].get_u32(); + self.state[operands.dst].set_u32(a.rotate_right(b)); + ControlFlow::Continue(()) + } + + fn xrotr64(&mut self, operands: BinaryOperands) -> ControlFlow { + let a = self.state[operands.src1].get_u64(); + let b = self.state[operands.src2].get_u32(); + self.state[operands.dst].set_u64(a.rotate_right(b)); + ControlFlow::Continue(()) + } + fn xselect32( &mut self, dst: XReg, diff --git a/pulley/src/lib.rs b/pulley/src/lib.rs index 64d5f75020b2..bffb05c3484a 100644 --- a/pulley/src/lib.rs +++ b/pulley/src/lib.rs @@ -200,6 +200,21 @@ macro_rules! for_each_op { /// `dst = leading_zeros(src)` xclz64 = Xclz64 { dst: XReg, src: XReg }; + /// `low32(dst) = count_ones(low32(src))` + xpopcnt32 = Xpopcnt32 { dst: XReg, src: XReg }; + /// `dst = count_ones(src)` + xpopcnt64 = Xpopcnt64 { dst: XReg, src: XReg }; + + /// `low32(dst) = rotate_left(low32(src1), low32(src2))` + xrotl32 = Xrotl32 { operands: BinaryOperands }; + /// `dst = rotate_left(src1, src2)` + xrotl64 = Xrotl64 { operands: BinaryOperands }; + + /// `low32(dst) = rotate_right(low32(src1), low32(src2))` + xrotr32 = Xrotr32 { operands: BinaryOperands }; + /// `dst = rotate_right(src1, src2)` + xrotr64 = Xrotr64 { operands: BinaryOperands }; + /// `low32(dst) = low32(src1) << low5(src2)` xshl32 = Xshl32 { operands: BinaryOperands }; /// `low32(dst) = low32(src1) >> low5(src2)`