Repository: bytecodealliance/wasmtime
Pull request #8685 (2024-05-23 00:44:38)

winch: Ensure the right return type for f64 comparisons
Fixes: https://github.com/bytecodealliance/wasmtime/issues/8632

This commit fixes the handling of f64 comparison operations in Winch. f64 comparison operations are defined as

	[f64 f64] -> [i32]

according to https://webassembly.github.io/spec/core/appendix/index-instructions.html

The previous implementation was widening the result to `[i64]`  which caused issues with stack shuffling in multi-value returns.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/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/codegen/src/prelude_opt.isle b/cranelift/codegen/src/prelude_opt.isle
index e0cd14860..8bd6370eb 100644
--- a/cranelift/codegen/src/prelude_opt.isle
+++ b/cranelift/codegen/src/prelude_opt.isle
@@ -74,7 +74,7 @@
 ;; When constructing, the rule will fail if the value cannot be represented in
 ;; the target type.  If it fits, it'll be masked accordingly in the constant.
 (decl iconst_s (Type i64) Value)
-(extractor (iconst_s ty c) (inst_data_tupled (iconst_sextend_etor ty c)))
+(extractor (iconst_s ty c) (sextend_maybe ty (inst_data_tupled (iconst_sextend_etor _inner_ty c))))
 (rule 0 (iconst_s ty c)
 	(if-let c_masked (u64_and (i64_as_u64 c) (ty_umax ty)))
 	(if-let c_reextended (i64_sextend_u64 ty c_masked))
@@ -89,7 +89,7 @@
 ;; When constructing, the rule will fail if the value cannot be represented in
 ;; the target type.
 (decl iconst_u (Type u64) Value)
-(extractor (iconst_u ty c) (iconst ty (u64_from_imm64 c)))
+(extractor (iconst_u ty c) (uextend_maybe ty (iconst _inner_ty (u64_from_imm64 c))))
 (rule 0 (iconst_u ty c)
 	(if-let $true (u64_le c (ty_umax ty)))
     (iconst ty (imm64 c)))
diff --git a/winch/codegen/src/codegen/context.rs b/winch/codegen/src/codegen/context.rs
index e613b1496..dbac5b613 100644
--- a/winch/codegen/src/codegen/context.rs
+++ b/winch/codegen/src/codegen/context.rs
@@ -261,8 +261,10 @@ impl<'a> CodeGenContext<'a> {
         self.free_reg(src2);
 
         let dst = match size {
-            OperandSize::S32 => TypedReg::i32(dst),
-            OperandSize::S64 => TypedReg::i64(dst),
+            // Float comparison operators are defined as
+            // [f64 f64] -> i32
+            // https://webassembly.github.io/spec/core/appendix/index-instructions.html
+            OperandSize::S32 | OperandSize::S64 => TypedReg::i32(dst),
             OperandSize::S8 | OperandSize::S16 | OperandSize::S128 => unreachable!(),
         };
         self.stack.push(dst.into());
diff --git a/winch/codegen/src/codegen/control.rs b/winch/codegen/src/codegen/control.rs
index 6c9e1ac0d..d7b7e87a9 100644
--- a/winch/codegen/src/codegen/control.rs
+++ b/winch/codegen/src/codegen/control.rs
@@ -608,7 +608,7 @@ impl ControlStackFrame {
         }
     }
 
-    /// Orchestrates how blocks results are handled.
+    /// Orchestrates how block results are handled.
     /// Results are handled in reverse order, starting from register results
     /// continuing to memory values. This guarantees that the stack ordering
     /// invariant is maintained. See [ABIResults] for more details.


Test patch:
diff --git a/cranelift/filetests/filetests/egraph/i128-opts.clif b/cranelift/filetests/filetests/egraph/i128-opts.clif
index f5347fb07..c84fc23bb 100644
--- a/cranelift/filetests/filetests/egraph/i128-opts.clif
+++ b/cranelift/filetests/filetests/egraph/i128-opts.clif
@@ -135,3 +135,23 @@ block0:
     ; check: v4 = iadd v1, v3
     ; check: return v4
 }
+
+function %mul_2u_i128(i128) -> i128 {
+block0(v0: i128):
+    v1 = iconst.i16 2
+    v2 = uextend.i128 v1
+    v3 = imul v0, v2
+    return v3
+    ; check: v4 = iadd v0, v0
+    ; check: return v4
+}
+
+function %mul_neg1_i128(i128) -> i128 {
+block0(v0: i128):
+    v1 = iconst.i16 -1
+    v2 = sextend.i128 v1
+    v3 = imul v0, v2
+    return v3
+    ; check: v4 = ineg v0
+    ; check: return v4
+}
diff --git a/tests/misc_testsuite/winch/float-comparison.wast b/tests/misc_testsuite/winch/float-comparison.wast
new file mode 100644
index 000000000..2e730847a
--- /dev/null
+++ b/tests/misc_testsuite/winch/float-comparison.wast
@@ -0,0 +1,13 @@
+(module
+  (func (result i32 i32 i32)
+    i32.const 1
+    i32.eqz
+    f64.const 0
+    f64.const 1
+    f64.ne
+    i32.const 1111
+  )
+  (export "d" (func 0))
+)
+
+(assert_return (invoke "d") (i32.const 0) (i32.const 1) (i32.const 1111))