LLVM entries take a Void parameter in the byte Cranelift passes

Cranelift gives a Void parameter an i8 register; the LLVM entry
convention declared it as an empty struct, which takes none, so a
Cranelift caller entering LLVM code (or LLVM calling Cranelift code
through a cell) shifted every later argument by one register. A region
outlined at a header carrying a void phi hit this once promoted.

git-bug: 5ee91b7ad5f22701c646a028a90165bd85bb5e15de674e3b2c2c77e64c88febb

Author: darmie

crates/compiler/src/abi.rs

@@ -9,6 +9,8 @@
 //!
 //! * a scalar, a pointer, a vector and a function pointer travel as
 //!   themselves;
+//! * a `Void` parameter travels as a byte nothing reads, so the
+//!   parameters after it keep their registers;
 //! * a struct of one scalar field travels as that scalar;
 //! * any other struct, and an array, travels as the address of its
 //!   storage;

crates/compiler/src/llvm_backend.rs

@@ -642,9 +642,12 @@ impl<'ctx> LLVMBackend<'ctx> {
     }
 
     /// The register form of a value of `ty` that travels as itself: a
-    /// struct of one scalar field is that scalar.
+    /// struct of one scalar field is that scalar, and `Void` the byte
+    /// Cranelift passes for it, which nothing reads. Without that byte
+    /// every later parameter would sit one register early.
     fn direct_type(&self, ty: &HirType) -> CompilerResult<BasicTypeEnum<'ctx>> {
         match ty {
+            HirType::Void => Ok(self.context.i8_type().into()),
             HirType::Struct(s) => match crate::abi::struct_carried_as_its_field(s) {
                 Some(field) => self.translate_type(field),
                 None => self.translate_type(ty),
@@ -1058,6 +1061,9 @@ impl<'ctx> LLVMBackend<'ctx> {
                 {
                     self.wrap_in_struct(raw, &param.ty)?
                 }
+                // The body holds a `Void` as the empty struct, whatever
+                // the parameter arrived as.
+                HirType::Void => self.translate_type(&param.ty)?.const_zero(),
                 _ => raw,
             };
             params.push(value);
@@ -1462,7 +1468,13 @@ impl<'ctx> LLVMBackend<'ctx> {
             let want: BasicTypeEnum<'ctx> = (*want).try_into().map_err(|_| {
                 CompilerError::CodeGen(format!("OSR re-entry parameter {i} is not a value"))
             })?;
-            let Some(slot) = slots.get(i).copied().flatten() else {
+            // Nothing reads a `Void`, so none is loaded from the frame.
+            let Some(slot) = slots
+                .get(i)
+                .copied()
+                .flatten()
+                .filter(|slot| layout.live_in_types.get(*slot) != Some(&HirType::Void))
+            else {
                 args.push(want.const_zero().into());
                 continue;
             };
@@ -4776,6 +4788,10 @@ impl<'ctx> LLVMBackend<'ctx> {
             None => None,
         };
         for ((arg, ty), pass) in args.iter().zip(&callee.params).zip(&callee.abi.params) {
+            if *pass == Pass::Direct && *ty == HirType::Void {
+                arg_values.push(self.direct_type(ty)?.const_zero().into());
+                continue;
+            }
             let value = self.get_value(*arg)?;
             match pass {
                 Pass::Pointer => {

crates/compiler/tests/void_param_abi.rs

@@ -0,0 +1,194 @@
+//! A `Void` parameter keeps the parameters after it where the other
+//! tier looks for them.
+//!
+//! Cranelift passes a `Void` as a byte in its own register. An LLVM
+//! entry standing in a call cell that Cranelift callers read, and an
+//! LLVM call through a cell holding Cranelift's code, must pass it the
+//! same way, or every later argument arrives in the register of the
+//! one before it.
+
+#![cfg(all(feature = "cranelift-backend", feature = "llvm-backend"))]
+
+use std::collections::HashSet;
+use std::sync::Arc;
+use zyntax_compiler::hir::{
+    BinaryOp, HirBlock, HirCallable, HirConstant, HirFunction, HirFunctionSignature, HirId,
+    HirInstruction, HirModule, HirParam, HirTerminator, HirType, HirValue, HirValueKind,
+    ParamAttributes,
+};
+use zyntax_typed_ast::InternedString;
+
+fn sig(params: Vec<HirType>, returns: Vec<HirType>) -> HirFunctionSignature {
+    HirFunctionSignature {
+        params: params
+            .into_iter()
+            .enumerate()
+            .map(|(i, ty)| HirParam {
+                id: HirId::new(),
+                name: InternedString::new_global(&format!("p{i}")),
+                ty,
+                attributes: ParamAttributes::default(),
+                ownership: Default::default(),
+            })
+            .collect(),
+        returns,
+        type_params: vec![],
+        const_params: vec![],
+        lifetime_params: vec![],
+        is_variadic: false,
+        is_async: false,
+        is_fiber: false,
+        effects: vec![],
+        is_pure: false,
+    }
+}
+
+fn add_value(func: &mut HirFunction, ty: HirType, kind: HirValueKind) -> HirId {
+    let id = HirId::new();
+    func.values.insert(
+        id,
+        HirValue {
+            id,
+            ty,
+            kind,
+            uses: HashSet::new(),
+            span: None,
+        },
+    );
+    id
+}
+
+fn konst(func: &mut HirFunction, v: i64) -> HirId {
+    add_value(
+        func,
+        HirType::I64,
+        HirValueKind::Constant(HirConstant::I64(v)),
+    )
+}
+
+fn body(func: &mut HirFunction) -> &mut HirBlock {
+    let entry = func.entry_block;
+    func.blocks.get_mut(&entry).unwrap()
+}
+
+/// `def pick(a: i64, u: Void, b: i64, c: i64): i64 { return a*100 + b*10 + c }`
+fn build_pick() -> HirFunction {
+    let mut f = HirFunction::new(
+        InternedString::new_global("pick"),
+        sig(
+            vec![HirType::I64, HirType::Void, HirType::I64, HirType::I64],
+            vec![HirType::I64],
+        ),
+    );
+    let a = add_value(&mut f, HirType::I64, HirValueKind::Parameter(0));
+    let b = add_value(&mut f, HirType::I64, HirValueKind::Parameter(2));
+    let c = add_value(&mut f, HirType::I64, HirValueKind::Parameter(3));
+    let hundred = konst(&mut f, 100);
+    let ten = konst(&mut f, 10);
+    let v: Vec<HirId> = (0..4)
+        .map(|_| add_value(&mut f, HirType::I64, HirValueKind::Instruction))
+        .collect();
+    let arith = |op, result, left, right| HirInstruction::Binary {
+        op,
+        result,
+        ty: HirType::I64,
+        left,
+        right,
+    };
+    let blk = body(&mut f);
+    blk.instructions.extend([
+        arith(BinaryOp::Mul, v[0], a, hundred),
+        arith(BinaryOp::Mul, v[1], b, ten),
+        arith(BinaryOp::Add, v[2], v[0], v[1]),
+        arith(BinaryOp::Add, v[3], v[2], c),
+    ]);
+    blk.terminator = HirTerminator::Return { values: vec![v[3]] };
+    f
+}
+
+/// `def pick_main(): i64 { return pick(1, (), 2, 3) }`
+fn build_pick_main(pick_id: HirId) -> HirFunction {
+    let mut f = HirFunction::new(
+        InternedString::new_global("pick_main"),
+        sig(vec![], vec![HirType::I64]),
+    );
+    let args: Vec<HirId> = [1, 2, 3].into_iter().map(|v| konst(&mut f, v)).collect();
+    let unit = add_value(&mut f, HirType::Void, HirValueKind::Undef);
+    let r = add_value(&mut f, HirType::I64, HirValueKind::Instruction);
+    let blk = body(&mut f);
+    blk.instructions.push(HirInstruction::Call {
+        result: Some(r),
+        callee: HirCallable::Function(pick_id),
+        args: vec![args[0], unit, args[1], args[2]],
+        type_args: vec![],
+        const_args: vec![],
+        is_tail: false,
+    });
+    blk.terminator = HirTerminator::Return { values: vec![r] };
+    f
+}
+
+const PICK_EXPECTED: i64 = 123;
+
+fn pick_module() -> (HirModule, HirFunction, HirFunction) {
+    let pick = build_pick();
+    let main = build_pick_main(pick.id);
+    let mut module = HirModule::new(InternedString::new_global("void_param"));
+    module.functions.insert(pick.id, pick.clone());
+    module.functions.insert(main.id, main.clone());
+    (module, pick, main)
+}
+
+/// One call cell, entered first by Cranelift's code for `pick` and then
+/// by LLVM's, from the same Cranelift caller.
+#[test]
+fn llvm_entry_takes_the_parameters_after_a_void_where_cranelift_passes_them() {
+    use inkwell::context::Context;
+    use zyntax_compiler::cranelift_backend::CraneliftBackend;
+    use zyntax_compiler::llvm_jit_backend::LLVMJitBackend;
+
+    let (module, pick, main) = pick_module();
+    let mut cranelift = CraneliftBackend::new().expect("backend");
+    cranelift.set_reloadable_calls(true);
+    cranelift.compile_module(&module).expect("compile");
+    cranelift.finalize_definitions().expect("finalize");
+    let ptr = cranelift.get_function_ptr(main.id).expect("main compiled");
+    let run: unsafe extern "C" fn() -> i64 = unsafe { std::mem::transmute(ptr) };
+    assert_eq!(unsafe { run() }, PICK_EXPECTED);
+
+    let context = Context::create();
+    let mut llvm = LLVMJitBackend::new(&context).expect("backend");
+    llvm.set_use_mcjit(true);
+    llvm.set_module_context(Arc::new(module.clone()));
+    llvm.set_cross_tier_links(cranelift.reload_key(), Arc::new(|_| None));
+    llvm.compile_function(pick.id, &pick)
+        .expect("LLVM compiles the entry");
+    let entry = llvm.get_function_pointer(pick.id).expect("pick compiled");
+    cranelift.publish_call_target(pick.id, entry as usize);
+    assert_eq!(unsafe { run() }, PICK_EXPECTED);
+}
+
+/// An LLVM entry calling Cranelift's `pick` through its cell.
+#[test]
+fn llvm_passes_the_parameters_after_a_void_where_cranelift_takes_them() {
+    use inkwell::context::Context;
+    use zyntax_compiler::cranelift_backend::CraneliftBackend;
+    use zyntax_compiler::llvm_jit_backend::LLVMJitBackend;
+
+    let (module, _pick, main) = pick_module();
+    let mut cranelift = CraneliftBackend::new().expect("backend");
+    cranelift.set_reloadable_calls(true);
+    cranelift.compile_module(&module).expect("compile");
+    cranelift.finalize_definitions().expect("finalize");
+
+    let context = Context::create();
+    let mut llvm = LLVMJitBackend::new(&context).expect("backend");
+    llvm.set_use_mcjit(true);
+    llvm.set_module_context(Arc::new(module.clone()));
+    llvm.set_cross_tier_links(cranelift.reload_key(), Arc::new(|_| None));
+    llvm.compile_function(main.id, &main)
+        .expect("LLVM compiles the caller");
+    let entry = llvm.get_function_pointer(main.id).expect("main compiled");
+    let run: unsafe extern "C" fn() -> i64 = unsafe { std::mem::transmute(entry) };
+    assert_eq!(unsafe { run() }, PICK_EXPECTED);
+}

crates/zyntax_python/tests/fixtures/void_loop_value.py

@@ -0,0 +1,31 @@
+# A driver loop whose body binds the None a call returns, so a loop
+# value of no type crosses its header, in a function that returns the
+# list it fills. The interpreted frame leaves into the region outlined
+# for it once the optimizing tier has compiled the region, and the
+# parameters after that value must arrive where the region reads them.
+def work(k, acc):
+    s = acc[0]
+    for j in range(k):
+        s = (s * 31 + j) % 1000003
+    acc[0] = s
+
+
+def main(n):
+    acc = [1]
+    times = []
+    for i in range(n):
+        o = work(20000000 if i >= 60 else 1000, acc)
+        times.append(1.0 * i)
+    times.append(1.0 * acc[0])
+    return times
+
+
+def run(n, f):
+    data = f(n)
+    total = 0.0
+    for x in data:
+        total += x
+    print(len(data), total)
+
+
+run(70, main)

crates/zyntax_python/tests/resume_points.rs

@@ -275,3 +275,47 @@ fn a_site_asked_during_an_entry_count_promotion_gets_the_optimizing_tier() {
     }
     assert!(transfers > 0, "no frame moved to a resume point in 20 runs");
 }
+
+/// A frame leaves into a region the optimizing tier compiled, whose
+/// parameters include a loop value of no type: every parameter after it
+/// reaches the region where the region reads it, and the list the
+/// region returns is the one the frame filled. The frame asks while
+/// its loop is warm, which takes the warm-up worker; whether the region
+/// is promoted before the frame leaves is up to timing, so the program
+/// runs until it has been, each run answering as CPython does.
+#[cfg(feature = "llvm-backend")]
+#[test]
+fn a_region_taking_a_void_loop_value_gets_the_parameters_after_it() {
+    let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/void_loop_value.py");
+    let mut entered = false;
+    for _ in 0..5 {
+        let output = Command::new(env!("CARGO_BIN_EXE_zypy"))
+            .arg("run")
+            .arg(&script)
+            .env("ZYPY_LLVM", "1")
+            .env("ZYNTAX_OSR_TRACE", "1")
+            .env_remove("ZYNTAX_DISABLE_WARM_UP")
+            .output()
+            .expect("zypy starts");
+        let stdout = String::from_utf8_lossy(&output.stdout);
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        assert!(output.status.success(), "{stderr}");
+        assert_eq!(stdout.trim(), "71 900854.0", "{stderr}");
+        let site = stderr
+            .lines()
+            .find(|l| l.starts_with("[osr] main site=") && l.ends_with("outlined resume point"))
+            .and_then(|l| l.split("site=").nth(1))
+            .and_then(|s| s.split(':').next());
+        let promoted = stderr.find("(main$resume0) at tier 1");
+        if let (Some(site), Some(promoted)) = (site, promoted)
+            && stderr[promoted..].contains(&format!("interpreted frame leaves at site={site} "))
+        {
+            entered = true;
+            break;
+        }
+    }
+    assert!(
+        entered,
+        "in 5 runs the frame never entered the region after its promotion"
+    );
+}