From 24cc641cb51e1e9c9a0ac1403a295c62fcd5e0a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= Date: Fri, 28 Jun 2024 09:52:47 -0400 Subject: [PATCH] Save state before emitting `br_if` Fixes: https://github.com/bytecodealliance/wasmtime/issues/8848 Similar to all the control instructions, any state must be explicitly saved before emitting the code for `br_if`. This commit ensures that live locals and registers are explicilty saved before emitting the code for `br_if`. Prior to this commit, live locals and registers were not saved every time causing incorrect behavior in cases where the calculation of the conditional argument didn't trigger a spill. This change introduces the explicit spill after calculating the branch condition argument to minimize memory traffic in case the conditional is already in a register. --- .../br_if/save_state_before_br_emission.wat | 105 ++++++++++++++++++ winch/codegen/src/visitor.rs | 5 + 2 files changed, 110 insertions(+) create mode 100644 tests/disas/winch/x64/br_if/save_state_before_br_emission.wat diff --git a/tests/disas/winch/x64/br_if/save_state_before_br_emission.wat b/tests/disas/winch/x64/br_if/save_state_before_br_emission.wat new file mode 100644 index 000000000000..016361685b0b --- /dev/null +++ b/tests/disas/winch/x64/br_if/save_state_before_br_emission.wat @@ -0,0 +1,105 @@ +;;! target = "x86_64" +;;! test = "winch" +(module + (func (param f64 f64 f64 f64) (result f32 f64) + f64.const 0 + local.get 0 + i64.const 0 + f64.const 0 + i64.const 0 + local.get 0 + f64.const 0 + i64.const 1 + i32.const 1 + i64.const 1 + f32.const 0 + local.get 0 + + i32.const 0 + br_if 0 + + drop + drop + drop + drop + drop + drop + i64.reinterpret_f64 + i64.const 0 + i64.xor + drop + drop + drop + drop + drop + drop + f32.const 0 + f64.const 0 + ) +) +;; wasm[0]::function[0]: +;; pushq %rbp +;; movq %rsp, %rbp +;; movq 8(%rdi), %r11 +;; movq (%r11), %r11 +;; addq $0x50, %r11 +;; cmpq %rsp, %r11 +;; ja 0x101 +;; 1b: movq %rdi, %r14 +;; subq $0x38, %rsp +;; movq %rdi, 0x30(%rsp) +;; movq %rsi, 0x28(%rsp) +;; movsd %xmm0, 0x20(%rsp) +;; movsd %xmm1, 0x18(%rsp) +;; movsd %xmm2, 0x10(%rsp) +;; movsd %xmm3, 8(%rsp) +;; movq %rdx, (%rsp) +;; movl $0, %eax +;; movsd 0x20(%rsp), %xmm15 +;; subq $8, %rsp +;; movsd %xmm15, (%rsp) +;; movsd 0x28(%rsp), %xmm15 +;; subq $8, %rsp +;; movsd %xmm15, (%rsp) +;; movsd 0x30(%rsp), %xmm15 +;; subq $8, %rsp +;; movsd %xmm15, (%rsp) +;; movsd (%rsp), %xmm0 +;; addq $8, %rsp +;; subq $4, %rsp +;; movss 0x72(%rip), %xmm15 +;; movss %xmm15, (%rsp) +;; testl %eax, %eax +;; je 0xb6 +;; a4: movl (%rsp), %r11d +;; movl %r11d, 0x10(%rsp) +;; addq $0x10, %rsp +;; jmp 0xeb +;; b6: addq $4, %rsp +;; movsd (%rsp), %xmm0 +;; addq $8, %rsp +;; movq %xmm0, %rax +;; xorq $0, %rax +;; addq $8, %rsp +;; movsd 0x38(%rip), %xmm0 +;; subq $4, %rsp +;; movss 0x23(%rip), %xmm15 +;; movss %xmm15, (%rsp) +;; movq 4(%rsp), %rax +;; movl (%rsp), %r11d +;; addq $4, %rsp +;; movl %r11d, (%rax) +;; addq $0x38, %rsp +;; popq %rbp +;; retq +;; 101: ud2 +;; 103: addb %al, (%rax) +;; 105: addb %al, (%rax) +;; 107: addb %al, (%rax) +;; 109: addb %al, (%rax) +;; 10b: addb %al, (%rax) +;; 10d: addb %al, (%rax) +;; 10f: addb %al, (%rax) +;; 111: addb %al, (%rax) +;; 113: addb %al, (%rax) +;; 115: addb %al, (%rax) diff --git a/winch/codegen/src/visitor.rs b/winch/codegen/src/visitor.rs index 97b60cecdf9d..fae3189df8cf 100644 --- a/winch/codegen/src/visitor.rs +++ b/winch/codegen/src/visitor.rs @@ -1711,6 +1711,11 @@ where self.masm, |ctx, masm| ctx.pop_to_reg(masm, None), ); + // Explicitly save any live registers and locals before setting up + // the branch state. + // In some cases, calculating the `top` value above, will result in + // a spill, thus the following one will result in a no-op. + self.context.spill(self.masm); frame.top_abi_results::( &mut self.context, self.masm,