wasm backend: PR #691 review follow-ups (GuardSubclass width, IntSignext/indexed-GC decline, bridge-iter journal root)

majit/majit-backend-dynasm/src/x86/assembler.rs

@@ -1470,7 +1470,18 @@ impl<'a> Assembler386<'a> {
                 }
             }
             Loc::Immed(i) => {
-                let v = i.value as i32;
+                // regloc.py:456-464 — an immediate that does not fit in 32
+                // bits cannot use the imm32 form (the encoder would truncate
+                // it and the CPU sign-extend the low half, e.g. an
+                // 0xFFFF_FFFF_FFFF mask becoming an all-ones no-op);
+                // materialize it into the scratch register and retry as the
+                // reg-reg form.
+                let Ok(v) = i32::try_from(i.value) else {
+                    let scratch = crate::regloc::X86_64_SCRATCH_REG;
+                    dynasm!(self.mc ; .arch x64 ; mov Rq(scratch.value), QWORD i.value);
+                    self.emit_binop_reg_loc(opcode, dst_reg, &Loc::Reg(scratch));
+                    return;
+                };
                 match opcode {
                     OpCode::IntAdd | OpCode::IntAddOvf | OpCode::NurseryPtrIncrement => {
                         dynasm!(self.mc ; .arch x64 ; add Rq(dst_reg), v);
@@ -1487,15 +1498,12 @@ impl<'a> Assembler386<'a> {
                     OpCode::IntXor => {
                         dynasm!(self.mc ; .arch x64 ; xor Rq(dst_reg), v);
                     }
-                    OpCode::IntMul | OpCode::IntMulOvf if i32::try_from(i.value).is_ok() => {
+                    OpCode::IntMul | OpCode::IntMulOvf => {
                         // imul r64, r64, imm32 (sign-extended) — one instruction
                         // instead of materializing the constant into a scratch reg.
                         dynasm!(self.mc ; .arch x64 ; imul Rq(dst_reg), Rq(dst_reg), v);
                     }
-                    _ => {
-                        let scratch = crate::regloc::X86_64_SCRATCH_REG.value;
-                        dynasm!(self.mc ; .arch x64 ; mov Rq(scratch), QWORD i.value ; imul Rq(dst_reg), Rq(scratch));
-                    }
+                    _ => {}
                 }
             }
             _ => {}
@@ -2989,15 +2997,22 @@ impl<'a> Assembler386<'a> {
                     match (a0, src) {
                         (Loc::Reg(a), Loc::Reg(s)) => dynasm!(self.mc ; .arch x64
                             ; lea Rq(dst.value), [Rq(a.value) + Rq(s.value)]),
-                        (Loc::Reg(a), Loc::Immed(i)) => {
-                            let v = i.value as i32;
-                            dynasm!(self.mc ; .arch x64
-                                ; lea Rq(dst.value), [Rq(a.value) + v])
-                        }
-                        (Loc::Immed(i), Loc::Reg(s)) => {
-                            let v = i.value as i32;
-                            dynasm!(self.mc ; .arch x64
-                                ; lea Rq(dst.value), [Rq(s.value) + v])
+                        (Loc::Reg(a), Loc::Immed(i)) | (Loc::Immed(i), Loc::Reg(a)) => {
+                            // The `_consider_lea` route guarantees a fitting
+                            // disp32, but the `consider_binop_symm` fallback
+                            // reaches this arm with an arbitrary 64-bit
+                            // constant (regloc.py:456-464); materialize a wide
+                            // one into the scratch register and use the
+                            // base+index form.
+                            if let Ok(v) = i32::try_from(i.value) {
+                                dynasm!(self.mc ; .arch x64
+                                    ; lea Rq(dst.value), [Rq(a.value) + v])
+                            } else {
+                                let scratch = crate::regloc::X86_64_SCRATCH_REG.value;
+                                dynasm!(self.mc ; .arch x64
+                                    ; mov Rq(scratch), QWORD i.value
+                                    ; lea Rq(dst.value), [Rq(a.value) + Rq(scratch)])
+                            }
                         }
                         (Loc::Immed(i0), Loc::Immed(i1)) => {
                             let sum = i0.value.wrapping_add(i1.value);

majit/majit-backend-dynasm/tests/basic_loop.rs

@@ -672,3 +672,71 @@ fn test_guard_no_exception_and_always_fails_emit_side_exits() {
         "GUARD_ALWAYS_FAILS should side-exit unconditionally"
     );
 }
+
+#[test]
+fn test_int_binop_wide_immediate_is_not_truncated() {
+    // An immediate that does not fit in 32 bits must not use the imm32
+    // form: the encoder would truncate it and the CPU sign-extend the low
+    // half, so `x & 0xFFFF_FFFF_FFFF` degenerated to `x & -1` (a no-op).
+    // regloc.py:456-464 routes such immediates through the scratch register.
+    let mask = 0xFFFF_FFFF_FFFFi64;
+    let mut backend = DynasmBackend::new();
+    backend.attach_default_test_descrs();
+    let mut token = JitCellToken::new(45);
+
+    let inputargs = vec![InputArg::from_type(Type::Int, 0)];
+    let i0 = inputargs[0].opref();
+
+    let and_op = Op::new(OpCode::IntAnd, &[rb(i0), rb(OpRef::const_int(mask))]);
+    and_op.pos.set(OpRef::int_op(1));
+
+    let finish_op = Op::new(OpCode::Finish, &[rb(OpRef::int_op(1))]);
+    finish_op.pos.set(OpRef::void_op(2));
+    finish_op.set_fail_arg_types(vec![Type::Int]);
+    finish_op.setfailargs(vec![rb(OpRef::int_op(1))].into());
+
+    let ops_rc: Vec<Rc<Op>> = vec![Rc::new(and_op), Rc::new(finish_op)];
+    let result = backend.compile_loop(&inputargs, &ops_rc, &mut token);
+    assert!(result.is_ok(), "compile_loop failed: {:?}", result.err());
+
+    let frame = backend.execute_token(&token, &[Value::Int(-1)]);
+    assert!(backend.get_latest_descr(&frame).is_finish());
+    assert_eq!(
+        backend.get_int_value(&frame, 0),
+        mask,
+        "-1 & 0xFFFF_FFFF_FFFF must keep only the low 48 bits"
+    );
+}
+
+#[test]
+fn test_int_add_wide_immediate_is_not_truncated() {
+    // Twin of the AND test for the LEA-form `int_add` emitter: a 2^32
+    // addend truncated to imm32 would add 0.
+    let addend = 1i64 << 32;
+    let mut backend = DynasmBackend::new();
+    backend.attach_default_test_descrs();
+    let mut token = JitCellToken::new(46);
+
+    let inputargs = vec![InputArg::from_type(Type::Int, 0)];
+    let i0 = inputargs[0].opref();
+
+    let add_op = Op::new(OpCode::IntAdd, &[rb(i0), rb(OpRef::const_int(addend))]);
+    add_op.pos.set(OpRef::int_op(1));
+
+    let finish_op = Op::new(OpCode::Finish, &[rb(OpRef::int_op(1))]);
+    finish_op.pos.set(OpRef::void_op(2));
+    finish_op.set_fail_arg_types(vec![Type::Int]);
+    finish_op.setfailargs(vec![rb(OpRef::int_op(1))].into());
+
+    let ops_rc: Vec<Rc<Op>> = vec![Rc::new(add_op), Rc::new(finish_op)];
+    let result = backend.compile_loop(&inputargs, &ops_rc, &mut token);
+    assert!(result.is_ok(), "compile_loop failed: {:?}", result.err());
+
+    let frame = backend.execute_token(&token, &[Value::Int(7)]);
+    assert!(backend.get_latest_descr(&frame).is_finish());
+    assert_eq!(
+        backend.get_int_value(&frame, 0),
+        addend + 7,
+        "7 + 2^32 must not truncate the immediate"
+    );
+}

majit/majit-backend-wasm/src/codegen.rs

@@ -2359,20 +2359,20 @@ fn build_function(
                 // int_signext(val, num_bytes): sign-extend from num_bytes width
                 let vi = op.pos.get().raw();
                 if !OpRef::raw_is_constant(vi) {
-                    emit_resolve(&mut sink, constants, value_types, op.arg(0).to_opref());
-                    // num_bytes (arg(1)) is always a compile-time constant;
-                    // resolve it like every other emit-time const so a genuine
-                    // pool miss panics via missing_emit_const instead of silently
-                    // defaulting to 8 (which zeroes the shift and skips the
-                    // narrowing, passing an un-truncated integer through).
+                    // The static shift below needs num_bytes (arg(1)) as an
+                    // emit-time constant. A non-constant width is still a valid
+                    // IR shape — int_signext/ii>i is a two-operand blackhole op
+                    // and the cranelift backend resolves arg(1) as a runtime
+                    // operand — just one this backend does not lower, so decline
+                    // for interpreter fallback rather than aborting the compile.
                     let arg1 = op.arg(1).to_opref();
-                    let num_bytes = const_operand_value(constants, arg1).unwrap_or_else(|| {
-                        panic!(
-                            "wasm int_signext: num_bytes operand (raw={}) is not a \
-                             resolvable compile-time constant",
+                    let Some(num_bytes) = const_operand_value(constants, arg1) else {
+                        return Err(BackendError::Unsupported(format!(
+                            "wasm int_signext: non-constant num_bytes operand (raw={})",
                             arg1.raw()
-                        )
-                    });
+                        )));
+                    };
+                    emit_resolve(&mut sink, constants, value_types, op.arg(0).to_opref());
                     let shift = 64 - num_bytes * 8;
                     if shift > 0 && shift < 64 {
                         sink.i64_const(shift);
@@ -2679,8 +2679,11 @@ fn build_function(
             // length's high bits — a silent wrong value on wasm, where offset is
             // valid linear memory and does not trap. pyre models strings/unicode
             // as Array(Char) and routes these through the descr-driven
-            // GETARRAYITEM/ARRAYLEN paths, so no producer emits these ops; decline
-            // them (interpreter fallback) rather than ship a wrong hardcoded read.
+            // GETARRAYITEM/ARRAYLEN paths, so no producer emits these ops (verified
+            // with PYRE_DUMP_PERFN_JITCODE: a str-subscript / len / compare / find
+            // hot loop traces to GETARRAYITEM, never STRGETITEM). Decline them
+            // (interpreter fallback) rather than ship a descr-driven lowering that
+            // no trace exercises — a valid but untestable path here.
             OpCode::Strlen | OpCode::Unicodelen | OpCode::Strgetitem | OpCode::Unicodegetitem => {
                 return Err(BackendError::Unsupported(format!(
                     "wasm codegen: string/unicode direct-memory op {:?} (no descr-driven layout)",
@@ -2689,22 +2692,29 @@ fn build_function(
             }
 
             // ── GC memory ops ──
-            // GC_LOAD/GC_STORE and their indexed forms are produced only by the
-            // GC rewrite (majit-gc/src/rewrite.rs): the true semantics are
-            // offset=arg1, size=arg2 (load) / value=arg2, size=arg3 (store), with
-            // no FieldDescr attached. The wasm backend does not run the GC rewrite,
-            // so these never reach here. The prior lowering read a nonexistent
-            // field_offset_from_descr (→ 0) and, for GcStore, stored arg(1) (the
-            // offset operand) as the value — a silent miscompile. Panic loudly like
-            // LoadFromGcTable rather than emit a wrong memory access.
-            OpCode::GcLoadI
-            | OpCode::GcLoadR
-            | OpCode::GcLoadF
-            | OpCode::GcLoadIndexedI
+            // The indexed forms are also wired as real frontend blackhole ops
+            // (blackhole.rs `gc_load_indexed_{i,f}` / `gc_store_indexed_{i,f}`),
+            // so an llop/buffer trace can carry them into the backend. This
+            // backend has no descr-driven lowering for them, so decline
+            // (interpreter fallback) rather than aborting the whole compile.
+            OpCode::GcLoadIndexedI
             | OpCode::GcLoadIndexedR
             | OpCode::GcLoadIndexedF
-            | OpCode::GcStore
             | OpCode::GcStoreIndexed => {
+                return Err(BackendError::Unsupported(format!(
+                    "wasm codegen: indexed GC op {:?} (no descr-driven layout)",
+                    op.opcode
+                )));
+            }
+            // The bare GC_LOAD/GC_STORE forms are produced only by the GC rewrite
+            // (majit-gc/src/rewrite.rs): the true semantics are offset=arg1,
+            // size=arg2 (load) / value=arg2, size=arg3 (store), with no FieldDescr
+            // attached. The wasm backend does not run the GC rewrite, so these
+            // never reach here. The prior lowering read a nonexistent
+            // field_offset_from_descr (→ 0) and, for GcStore, stored arg(1) (the
+            // offset operand) as the value — a silent miscompile. Panic loudly like
+            // LoadFromGcTable rather than emit a wrong memory access.
+            OpCode::GcLoadI | OpCode::GcLoadR | OpCode::GcLoadF | OpCode::GcStore => {
                 panic!(
                     "wasm backend: {:?} is unsupported (GC_LOAD/GC_STORE); \
                      the GC rewrite must not run for wasm",
@@ -2934,10 +2944,14 @@ fn build_function(
                     sink.i32_wrap_i64();
                     sink.i64_load(mem64(vtable_off as u64));
                     sink.i32_wrap_i64();
+                    // subclassrange_min is an 8-byte i64 on every target
+                    // (pyobject.rs `PyType::subclassrange_min: AtomicI64`); read
+                    // the full field width, not the wasm32 4-byte `usize`, or the
+                    // guard truncates/sign-extends the object's min.
                     emit_sized_int_load(
                         &mut sink,
                         offset2 as u64,
-                        std::mem::size_of::<usize>(),
+                        std::mem::size_of::<i64>(),
                         true,
                     );
                 } else {
@@ -2963,7 +2977,8 @@ fn build_function(
                     sink.i64_const((guard_gc_type_info.sizeof_ti + offset2) as i64);
                     sink.i64_add();
                     sink.i32_wrap_i64();
-                    emit_sized_int_load(&mut sink, 0, std::mem::size_of::<usize>(), true);
+                    // 8-byte i64 subclassrange_min (see the vtable path above).
+                    emit_sized_int_load(&mut sink, 0, std::mem::size_of::<i64>(), true);
                 }
                 // Stack: [..., loc_tmp (i64)]