Repository: bytecodealliance/wasmtime
Pull request #5786 (2023-02-15 14:51:36)

s390x: Fix instruction encoding and disassembly format bugs
- Fix encoding of the AHY instruction.
- Fix disassembly format of FIEBR, FIDBR, and LEDBRA instructions.

CC @cfallin @elliottt - this fixes s390x issues detected here: https://github.com/bytecodealliance/wasmtime/pull/5780
<!--

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).
-->


afonso360: Hey, I'm trying to understand why fuzzgen didn't catch this when you ran it a few weeks ago. This encoding error only triggers when we try to add a 16bit value directly from memory to a 32 bit register right?

I think fuzzgen can already generate code like that, although we only ever load from the stack, does s390x select a different encoding in those cases?
afonso360: Oh! That explains it, We constrain the stack size quite a lot. We should probably increase that. Thanks!

Patch:
diff --git a/cranelift/codegen/src/isa/s390x/inst/emit.rs b/cranelift/codegen/src/isa/s390x/inst/emit.rs
index 23ea1e736..5a811e7a3 100644
--- a/cranelift/codegen/src/isa/s390x/inst/emit.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/emit.rs
@@ -1568,7 +1568,7 @@ impl Inst {
 
                 let (opcode_rx, opcode_rxy) = match alu_op {
                     ALUOp::Add32 => (Some(0x5a), Some(0xe35a)),        // A(Y)
-                    ALUOp::Add32Ext16 => (Some(0x4a), Some(0xe34a)),   // AH(Y)
+                    ALUOp::Add32Ext16 => (Some(0x4a), Some(0xe37a)),   // AH(Y)
                     ALUOp::Add64 => (None, Some(0xe308)),              // AG
                     ALUOp::Add64Ext16 => (None, Some(0xe338)),         // AGH
                     ALUOp::Add64Ext32 => (None, Some(0xe318)),         // AGF
diff --git a/cranelift/codegen/src/isa/s390x/inst/mod.rs b/cranelift/codegen/src/isa/s390x/inst/mod.rs
index 3804a980c..d68e3317c 100644
--- a/cranelift/codegen/src/isa/s390x/inst/mod.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/mod.rs
@@ -2359,11 +2359,16 @@ impl Inst {
                 let (rn, rn_fpr) = pretty_print_fpr(rn, allocs);
                 if opcode_fpr.is_some() && rd_fpr.is_some() && rn_fpr.is_some() {
                     format!(
-                        "{} {}, {}, {}",
+                        "{} {}, {}, {}{}",
                         opcode_fpr.unwrap(),
                         rd_fpr.unwrap(),
+                        mode,
                         rn_fpr.unwrap(),
-                        mode
+                        if opcode_fpr.unwrap().ends_with('a') {
+                            ", 0"
+                        } else {
+                            ""
+                        }
                     )
                 } else if opcode.starts_with('w') {
                     format!(


Test patch:
diff --git a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
index e2cb0e053..943762540 100644
--- a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
+++ b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs
@@ -587,7 +587,7 @@ fn test_s390x_binemit() {
                 flags: MemFlags::trusted(),
             },
         },
-        "E3102000004A",
+        "E3102000007A",
         "ahy %r1, 0(%r2)",
     ));
     insns.push((
@@ -7999,7 +7999,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B344008C",
-        "ledbra %f8, %f12, 0",
+        "ledbra %f8, 0, %f12, 0",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8029,7 +8029,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B357708C",
-        "fiebr %f8, %f12, 7",
+        "fiebr %f8, 7, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8039,7 +8039,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B35F708C",
-        "fidbr %f8, %f12, 7",
+        "fidbr %f8, 7, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8049,7 +8049,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B357608C",
-        "fiebr %f8, %f12, 6",
+        "fiebr %f8, 6, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8059,7 +8059,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B35F608C",
-        "fidbr %f8, %f12, 6",
+        "fidbr %f8, 6, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8069,7 +8069,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B357508C",
-        "fiebr %f8, %f12, 5",
+        "fiebr %f8, 5, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8079,7 +8079,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B35F508C",
-        "fidbr %f8, %f12, 5",
+        "fidbr %f8, 5, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8089,7 +8089,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B357408C",
-        "fiebr %f8, %f12, 4",
+        "fiebr %f8, 4, %f12",
     ));
     insns.push((
         Inst::FpuRound {
@@ -8099,7 +8099,7 @@ fn test_s390x_binemit() {
             rn: vr(12),
         },
         "B35F408C",
-        "fidbr %f8, %f12, 4",
+        "fidbr %f8, 4, %f12",
     ));
     insns.push((
         Inst::FpuRound {
diff --git a/cranelift/filetests/filetests/isa/s390x/floating-point-arch13.clif b/cranelift/filetests/filetests/isa/s390x/floating-point-arch13.clif
index dadca7c76..9bddd0413 100644
--- a/cranelift/filetests/filetests/isa/s390x/floating-point-arch13.clif
+++ b/cranelift/filetests/filetests/isa/s390x/floating-point-arch13.clif
@@ -386,7 +386,7 @@ block0(v0: i64):
 ; block0:
 ;   ldgr %f2, %r2
 ;   wcdlgb %f4, %f2, 0, 3
-;   ledbra %f0, %f4, 4
+;   ledbra %f0, 4, %f4, 0
 ;   br %r14
 
 function %fcvt_from_sint_i64_f32(i64) -> f32 {
@@ -398,7 +398,7 @@ block0(v0: i64):
 ; block0:
 ;   ldgr %f2, %r2
 ;   wcdgb %f4, %f2, 0, 3
-;   ledbra %f0, %f4, 4
+;   ledbra %f0, 4, %f4, 0
 ;   br %r14
 
 function %fcvt_from_uint_i8_f64(i8) -> f64 {
diff --git a/cranelift/filetests/filetests/isa/s390x/floating-point.clif b/cranelift/filetests/filetests/isa/s390x/floating-point.clif
index 4a03009a5..eee92024b 100644
--- a/cranelift/filetests/filetests/isa/s390x/floating-point.clif
+++ b/cranelift/filetests/filetests/isa/s390x/floating-point.clif
@@ -285,7 +285,7 @@ block0(v0: f64):
 }
 
 ; block0:
-;   ledbra %f0, %f0, 0
+;   ledbra %f0, 0, %f0, 0
 ;   br %r14
 
 function %ceil_f32(f32) -> f32 {
@@ -295,7 +295,7 @@ block0(v0: f32):
 }
 
 ; block0:
-;   fiebr %f0, %f0, 6
+;   fiebr %f0, 6, %f0
 ;   br %r14
 
 function %ceil_f64(f64) -> f64 {
@@ -305,7 +305,7 @@ block0(v0: f64):
 }
 
 ; block0:
-;   fidbr %f0, %f0, 6
+;   fidbr %f0, 6, %f0
 ;   br %r14
 
 function %floor_f32(f32) -> f32 {
@@ -315,7 +315,7 @@ block0(v0: f32):
 }
 
 ; block0:
-;   fiebr %f0, %f0, 7
+;   fiebr %f0, 7, %f0
 ;   br %r14
 
 function %floor_f64(f64) -> f64 {
@@ -325,7 +325,7 @@ block0(v0: f64):
 }
 
 ; block0:
-;   fidbr %f0, %f0, 7
+;   fidbr %f0, 7, %f0
 ;   br %r14
 
 function %trunc_f32(f32) -> f32 {
@@ -335,7 +335,7 @@ block0(v0: f32):
 }
 
 ; block0:
-;   fiebr %f0, %f0, 5
+;   fiebr %f0, 5, %f0
 ;   br %r14
 
 function %trunc_f64(f64) -> f64 {
@@ -345,7 +345,7 @@ block0(v0: f64):
 }
 
 ; block0:
-;   fidbr %f0, %f0, 5
+;   fidbr %f0, 5, %f0
 ;   br %r14
 
 function %nearest_f32(f32) -> f32 {
@@ -355,7 +355,7 @@ block0(v0: f32):
 }
 
 ; block0:
-;   fiebr %f0, %f0, 4
+;   fiebr %f0, 4, %f0
 ;   br %r14
 
 function %nearest_f64(f64) -> f64 {
@@ -365,7 +365,7 @@ block0(v0: f64):
 }
 
 ; block0:
-;   fidbr %f0, %f0, 4
+;   fidbr %f0, 4, %f0
 ;   br %r14
 
 function %fma_f32(f32, f32, f32) -> f32 {
@@ -732,7 +732,7 @@ block0(v0: i8):
 ;   llgcr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdlgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_sint_i8_f32(i8) -> f32 {
@@ -745,7 +745,7 @@ block0(v0: i8):
 ;   lgbr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_uint_i16_f32(i16) -> f32 {
@@ -758,7 +758,7 @@ block0(v0: i16):
 ;   llghr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdlgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_sint_i16_f32(i16) -> f32 {
@@ -771,7 +771,7 @@ block0(v0: i16):
 ;   lghr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_uint_i32_f32(i32) -> f32 {
@@ -784,7 +784,7 @@ block0(v0: i32):
 ;   llgfr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdlgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_sint_i32_f32(i32) -> f32 {
@@ -797,7 +797,7 @@ block0(v0: i32):
 ;   lgfr %r4, %r2
 ;   ldgr %f4, %r4
 ;   wcdgb %f6, %f4, 0, 3
-;   ledbra %f0, %f6, 4
+;   ledbra %f0, 4, %f6, 0
 ;   br %r14
 
 function %fcvt_from_uint_i64_f32(i64) -> f32 {
@@ -809,7 +809,7 @@ block0(v0: i64):
 ; block0:
 ;   ldgr %f2, %r2
 ;   wcdlgb %f4, %f2, 0, 3
-;   ledbra %f0, %f4, 4
+;   ledbra %f0, 4, %f4, 0
 ;   br %r14
 
 function %fcvt_from_sint_i64_f32(i64) -> f32 {
@@ -821,7 +821,7 @@ block0(v0: i64):
 ; block0:
 ;   ldgr %f2, %r2
 ;   wcdgb %f4, %f2, 0, 3
-;   ledbra %f0, %f4, 4
+;   ledbra %f0, 4, %f4, 0
 ;   br %r14
 
 function %fcvt_from_uint_i8_f64(i8) -> f64 {