Repository: bytecodealliance/wasmtime
Pull request #5703 (2023-02-03 20:26:56)

winch: Add missing conversions between x64 types
This commit adds some missing conversions between Winch's x64 `Reg` type and Cranelift's `Gpr`, `WritableGpr` and `GprMemImm`. This results in less boilerplate. This is also a bit of groundwork in the assembler to support the rest of the integer binary instructions.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->


github-actions[bot]: #### Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

* saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

[Learn more.](https://github.com/bytecodealliance/subscribe-to-label-action)
</details>

Patch:
diff --git a/cranelift/interpreter/src/interpreter.rs b/cranelift/interpreter/src/interpreter.rs
index 85f9fca53..7a5c00a16 100644
--- a/cranelift/interpreter/src/interpreter.rs
+++ b/cranelift/interpreter/src/interpreter.rs
@@ -125,6 +125,11 @@ impl<'a> Interpreter<'a> {
                         .set_all(function.dfg.inst_results(inst), returned_arguments);
                     maybe_inst = layout.next_inst(inst)
                 }
+                ControlFlow::ReturnCall(callee, args) => {
+                    self.state.pop_frame();
+                    let rets = self.call(callee, &args)?.unwrap_return();
+                    return Ok(ControlFlow::Return(rets.into()));
+                }
                 ControlFlow::Return(returned_values) => {
                     self.state.pop_frame();
                     return Ok(ControlFlow::Return(returned_values));
diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs
index 1bee1a2d1..78868f2af 100644
--- a/cranelift/interpreter/src/step.rs
+++ b/cranelift/interpreter/src/step.rs
@@ -34,6 +34,15 @@ fn validate_signature_params(sig: &[AbiParam], args: &[impl Value]) -> bool {
         })
 }
 
+// Helper for summing a sequence of values.
+fn sum<V: Value>(head: V, tail: SmallVec<[V; 1]>) -> ValueResult<i128> {
+    let mut acc = head;
+    for t in tail {
+        acc = Value::add(acc, t)?;
+    }
+    acc.into_int()
+}
+
 /// Interpret a single Cranelift instruction. Note that program traps and interpreter errors are
 /// distinct: a program trap results in `Ok(Flow::Trap(...))` whereas an interpretation error (e.g.
 /// the types of two values are incompatible) results in `Err(...)`.
@@ -267,14 +276,75 @@ where
         }
     };
 
-    // Helper for summing a sequence of values.
-    fn sum<V: Value>(head: V, tail: SmallVec<[V; 1]>) -> ValueResult<i128> {
-        let mut acc = head;
-        for t in tail {
-            acc = Value::add(acc, t)?;
+    // Perform a call operation.
+    //
+    // The returned `ControlFlow` variant is determined by the given function
+    // argument, which should make either a `ControlFlow::Call` or a
+    // `ControlFlow::ReturnCall`.
+    let do_call = |make_ctrl_flow: fn(&'a Function, SmallVec<[V; 1]>) -> ControlFlow<'a, V>|
+     -> Result<ControlFlow<'a, V>, StepError> {
+        let func_ref = if let InstructionData::Call { func_ref, .. } = inst {
+            func_ref
+        } else {
+            unreachable!()
+        };
+
+        let curr_func = state.get_current_function();
+        let ext_data = curr_func
+            .dfg
+            .ext_funcs
+            .get(func_ref)
+            .ok_or(StepError::UnknownFunction(func_ref))?;
+
+        let signature = if let Some(sig) = curr_func.dfg.signatures.get(ext_data.signature) {
+            sig
+        } else {
+            return Ok(ControlFlow::Trap(CraneliftTrap::User(
+                TrapCode::BadSignature,
+            )));
+        };
+
+        let args = args()?;
+
+        // Check the types of the arguments. This is usually done by the verifier, but nothing
+        // guarantees that the user has ran that.
+        let args_match = validate_signature_params(&signature.params[..], &args[..]);
+        if !args_match {
+            return Ok(ControlFlow::Trap(CraneliftTrap::User(
+                TrapCode::BadSignature,
+            )));
         }
-        acc.into_int()
-    }
+
+        Ok(match ext_data.name {
+            // These functions should be registered in the regular function store
+            ExternalName::User(_) | ExternalName::TestCase(_) => {
+                let function = state
+                    .get_function(func_ref)
+                    .ok_or(StepError::UnknownFunction(func_ref))?;
+
+                make_ctrl_flow(function, args)
+            }
+            ExternalName::LibCall(libcall) => {
+                debug_assert_ne!(inst.opcode(), Opcode::ReturnCall, "Cannot tail call to libcalls");
+                let libcall_handler = state.get_libcall_handler();
+
+                // We don't transfer control to a libcall, we just execute it and return the results
+                let res = libcall_handler(libcall, args);
+                let res = match res {
+                    Err(trap) => return Ok(ControlFlow::Trap(CraneliftTrap::User(trap))),
+                    Ok(rets) => rets,
+                };
+
+                // Check that what the handler returned is what we expect.
+                if validate_signature_params(&signature.returns[..], &res[..]) {
+                    ControlFlow::Assign(res)
+                } else {
+                    ControlFlow::Trap(CraneliftTrap::User(TrapCode::BadSignature))
+                }
+            }
+            ExternalName::KnownSymbol(_) => unimplemented!(),
+        })
+    };
 
     // Interpret a Cranelift instruction.
     Ok(match inst.opcode() {
@@ -328,70 +398,9 @@ where
         Opcode::Trapnz => trap_when(arg(0)?.into_bool()?, CraneliftTrap::User(trap_code())),
         Opcode::ResumableTrapnz => trap_when(arg(0)?.into_bool()?, CraneliftTrap::Resumable),
         Opcode::Return => ControlFlow::Return(args()?),
-        Opcode::Call => {
-            let func_ref = if let InstructionData::Call { func_ref, .. } = inst {
-                func_ref
-            } else {
-                unreachable!()
-            };
-
-            let curr_func = state.get_current_function();
-            let ext_data = curr_func
-                .dfg
-                .ext_funcs
-                .get(func_ref)
-                .ok_or(StepError::UnknownFunction(func_ref))?;
-
-            let signature = if let Some(sig) = curr_func.dfg.signatures.get(ext_data.signature) {
-                sig
-            } else {
-                return Ok(ControlFlow::Trap(CraneliftTrap::User(
-                    TrapCode::BadSignature,
-                )));
-            };
-
-            let args = args()?;
-
-            // Check the types of the arguments. This is usually done by the verifier, but nothing
-            // guarantees that the user has ran that.
-            let args_match = validate_signature_params(&signature.params[..], &args[..]);
-            if !args_match {
-                return Ok(ControlFlow::Trap(CraneliftTrap::User(
-                    TrapCode::BadSignature,
-                )));
-            }
-
-            match ext_data.name {
-                // These functions should be registered in the regular function store
-                ExternalName::User(_) | ExternalName::TestCase(_) => {
-                    let function = state
-                        .get_function(func_ref)
-                        .ok_or(StepError::UnknownFunction(func_ref))?;
-
-                    ControlFlow::Call(function, args)
-                }
-                ExternalName::LibCall(libcall) => {
-                    let libcall_handler = state.get_libcall_handler();
-
-                    // We don't transfer control to a libcall, we just execute it and return the results
-                    let res = libcall_handler(libcall, args);
-                    let res = match res {
-                        Err(trap) => return Ok(ControlFlow::Trap(CraneliftTrap::User(trap))),
-                        Ok(rets) => rets,
-                    };
-
-                    // Check that what the handler returned is what we expect.
-                    if validate_signature_params(&signature.returns[..], &res[..]) {
-                        ControlFlow::Assign(res)
-                    } else {
-                        ControlFlow::Trap(CraneliftTrap::User(TrapCode::BadSignature))
-                    }
-                }
-                ExternalName::KnownSymbol(_) => unimplemented!(),
-            }
-        }
+        Opcode::Call => do_call(ControlFlow::Call)?,
         Opcode::CallIndirect => unimplemented!("CallIndirect"),
-        Opcode::ReturnCall => unimplemented!("ReturnCall"),
+        Opcode::ReturnCall => do_call(ControlFlow::ReturnCall)?,
         Opcode::ReturnCallIndirect => unimplemented!("ReturnCallIndirect"),
         Opcode::FuncAddr => unimplemented!("FuncAddr"),
         Opcode::Load
@@ -1288,6 +1297,8 @@ pub enum ControlFlow<'a, V> {
     ContinueAt(Block, SmallVec<[V; 1]>),
     /// Indicates a call the given [Function] with the supplied arguments.
     Call(&'a Function, SmallVec<[V; 1]>),
+    /// Indicates a tail call to the given [Function] with the supplied arguments.
+    ReturnCall(&'a Function, SmallVec<[V; 1]>),
     /// Return from the current function with the given parameters, e.g.: `return [v1, v2]`.
     Return(SmallVec<[V; 1]>),
     /// Stop with a program-generated trap; note that these are distinct from errors that may occur
diff --git a/winch/codegen/src/isa/x64/asm.rs b/winch/codegen/src/isa/x64/asm.rs
index 6734a35d4..762885160 100644
--- a/winch/codegen/src/isa/x64/asm.rs
+++ b/winch/codegen/src/isa/x64/asm.rs
@@ -33,6 +33,25 @@ impl From<Reg> for RegMemImm {
     }
 }
 
+impl From<Reg> for WritableGpr {
+    fn from(reg: Reg) -> Self {
+        let writable = Writable::from_reg(reg.into());
+        WritableGpr::from_writable_reg(writable).expect("valid writable gpr")
+    }
+}
+
+impl From<Reg> for Gpr {
+    fn from(reg: Reg) -> Self {
+        Gpr::new(reg.into()).expect("valid gpr")
+    }
+}
+
+impl From<Reg> for GprMemImm {
+    fn from(reg: Reg) -> Self {
+        GprMemImm::new(reg.into()).expect("valid gpr")
+    }
+}
+
 impl From<OperandSize> for args::OperandSize {
     fn from(size: OperandSize) -> Self {
         match size {
@@ -74,8 +93,7 @@ impl Assembler {
 
     /// Push register.
     pub fn push_r(&mut self, reg: Reg) {
-        let src = GprMemImm::new(reg.into()).expect("valid gpr");
-        self.emit(Inst::Push64 { src });
+        self.emit(Inst::Push64 { src: reg.into() });
     }
 
     /// Pop to register.
@@ -118,25 +136,20 @@ impl Assembler {
 
     /// Register-to-register move.
     pub fn mov_rr(&mut self, src: Reg, dst: Reg, size: OperandSize) {
-        let src = Gpr::new(src.into()).expect("valid gpr");
-        let dst = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
-
         self.emit(Inst::MovRR {
-            src,
-            dst,
+            src: src.into(),
+            dst: dst.into(),
             size: size.into(),
         });
     }
 
     /// Register-to-memory move.
     pub fn mov_rm(&mut self, src: Reg, base: Reg, disp: u32, size: OperandSize) {
-        let src = Gpr::new(src.into()).expect("valid gpr");
         let dst = Amode::imm_reg(disp, base.into());
 
         self.emit(Inst::MovRM {
             size: size.into(),
-            src,
+            src: src.into(),
             dst: SyntheticAmode::real(dst),
         });
     }
@@ -167,37 +180,34 @@ impl Assembler {
     pub fn mov_mr(&mut self, base: Reg, disp: u32, dst: Reg, size: OperandSize) {
         use OperandSize::S64;
 
-        let dst = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
         let amode = Amode::imm_reg(disp, base.into());
         let src = SyntheticAmode::real(amode);
 
         if size == S64 {
-            self.emit(Inst::Mov64MR { src, dst });
+            self.emit(Inst::Mov64MR {
+                src,
+                dst: dst.into(),
+            });
         } else {
             let reg_mem = RegMem::mem(src);
             self.emit(Inst::MovzxRmR {
                 ext_mode: ExtMode::LQ,
                 src: GprMem::new(reg_mem).expect("valid memory address"),
-                dst,
+                dst: dst.into(),
             });
         }
     }
 
     /// Subtact immediate register.
-    pub fn sub_ir(&mut self, imm: u32, dst: Reg, size: OperandSize) {
-        let writable = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
-        let src = Gpr::new(dst.into()).expect("valid gpr");
-
-        let imm = RegMemImm::imm(imm);
+    pub fn sub_ir(&mut self, imm: i32, dst: Reg, size: OperandSize) {
+        let imm = RegMemImm::imm(imm as u32);
 
         self.emit(Inst::AluRmiR {
             size: size.into(),
             op: AluRmiROpcode::Sub,
-            src1: src,
+            src1: dst.into(),
             src2: GprMemImm::new(imm).expect("valid immediate"),
-            dst: writable,
+            dst: dst.into(),
         });
     }
 
@@ -215,52 +225,36 @@ impl Assembler {
 
     /// Add immediate and register.
     pub fn add_ir(&mut self, imm: i32, dst: Reg, size: OperandSize) {
-        let writable = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
-        let src = Gpr::new(dst.into()).expect("valid gpr");
-
         let imm = RegMemImm::imm(imm as u32);
 
         self.emit(Inst::AluRmiR {
             size: size.into(),
             op: AluRmiROpcode::Add,
-            src1: src,
+            src1: dst.into(),
             src2: GprMemImm::new(imm).expect("valid immediate"),
-            dst: writable,
+            dst: dst.into(),
         });
     }
 
     /// Add register and register.
     pub fn add_rr(&mut self, src: Reg, dst: Reg, size: OperandSize) {
-        let dest = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
-        let src1 = Gpr::new(dst.into()).expect("valid gpr");
-
-        let src2 = RegMemImm::reg(src.into());
-
         self.emit(Inst::AluRmiR {
             size: size.into(),
             op: AluRmiROpcode::Add,
-            src1,
-            src2: GprMemImm::new(src2).expect("valid gpr"),
-            dst: dest,
+            src1: dst.into(),
+            src2: src.into(),
+            dst: dst.into(),
         });
     }
 
     /// Logical exclusive or with registers.
     pub fn xor_rr(&mut self, src: Reg, dst: Reg, size: OperandSize) {
-        let dest = WritableGpr::from_writable_reg(Writable::from_reg(dst.into()))
-            .expect("valid writable gpr");
-        let src1 = Gpr::new(dst.into()).expect("valid gpr");
-
-        let src2 = RegMemImm::reg(src.into());
-
         self.emit(Inst::AluRmiR {
             size: size.into(),
             op: AluRmiROpcode::Xor,
-            src1,
-            src2: GprMemImm::new(src2).expect("valid gpr"),
-            dst: dest,
+            src1: dst.into(),
+            src2: src.into(),
+            dst: dst.into(),
         });
     }
 }
diff --git a/winch/codegen/src/isa/x64/masm.rs b/winch/codegen/src/isa/x64/masm.rs
index beb1481b5..b5cd3e54d 100644
--- a/winch/codegen/src/isa/x64/masm.rs
+++ b/winch/codegen/src/isa/x64/masm.rs
@@ -66,7 +66,7 @@ impl Masm for MacroAssembler {
             return;
         }
 
-        self.asm.sub_ir(bytes, rsp(), OperandSize::S64);
+        self.asm.sub_ir(bytes as i32, rsp(), OperandSize::S64);
         self.increment_sp(bytes);
     }
 


Test patch:
diff --git a/cranelift/filetests/filetests/runtests/return-call.clif b/cranelift/filetests/filetests/runtests/return-call.clif
new file mode 100644
index 000000000..4f317a41e
--- /dev/null
+++ b/cranelift/filetests/filetests/runtests/return-call.clif
@@ -0,0 +1,68 @@
+test interpret
+;; test run
+;; target x86_64
+;; target aarch64
+;; target aarch64 sign_return_address
+;; target aarch64 has_pauth sign_return_address
+;; target s390x
+
+;;;; Test passing `i64`s ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+function %callee_i64(i64) -> i64 tail {
+block0(v0: i64):
+    v1 = iadd_imm.i64 v0, 10
+    return v1
+}
+
+function %call_i64(i64) -> i64 tail {
+    fn0 = %callee_i64(i64) -> i64 tail
+
+block0(v0: i64):
+    return_call fn0(v0)
+}
+; run: %call_i64(10) == 20
+
+;;;; Test colocated tail calls ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+function %colocated_i64(i64) -> i64 tail {
+    fn0 = colocated %callee_i64(i64) -> i64 tail
+
+block0(v0: i64):
+    return_call fn0(v0)
+}
+; run: %colocated_i64(10) == 20
+
+;;;; Test passing `f64`s ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+function %callee_f64(f64) -> f64 tail {
+block0(v0: f64):
+    v1 = f64const 0x10.0
+    v2 = fadd.f64 v0, v1
+    return v2
+}
+
+function %call_f64(f64) -> f64 tail {
+    fn0 = %callee_f64(f64) -> f64 tail
+
+block0(v0: f64):
+    return_call fn0(v0)
+}
+; run: %call_f64(0x10.0) == 0x20.0
+
+;;;; Test passing `i8`s ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+function %callee_i8(i8) -> i8 tail {
+block0(v0: i8):
+    v1 = iconst.i8 0
+    v2 = icmp eq v0, v1
+    return v2
+}
+
+function %call_i8(i8) -> i8 tail {
+    fn0 = %callee_i8(i8) -> i8 tail
+
+block0(v0: i8):
+    return_call fn0(v0)
+}
+; run: %call_i8(1) == 0
+; run: %call_i8(0) == 1