Repository: bytecodealliance/wasmtime
Pull request #8508 (2024-04-29 21:30:38)

cranelift/x64: Fix XmmRmREvex pretty-printing
The operand collector had these operands in src1/src2/dst order, but the pretty-printer had dst/src1/src2 order instead.

Note that fixing the pretty-printer makes it disagree with the disassembly from Capstone. I have stared at the emit code and the Intel reference manual and can't figure out how to reconcile these.

However, I have verified that the `vpsraq` instruction is executed on my laptop in a runtest (`cranelift/filetests/filetests/runtests/simd-sshr.clif`), and its runtime behavior matches the CLIF interpreter. So this does not appear to be a codegen/correctness bug.

I'm hoping @abrown or @alexcrichton can help explain the disassembly discrepancy.

jameysharp: > * why does changing the order of pretty-print invocation matter here? Is that thing still stateful?

Right now, the order that the backend's implementation of `get_operands` invokes methods like `collector.reg_use` must match the order that `pretty_print` or `emit` invokes methods like `allocs.next`. The first puts operands into the input for regalloc2, and the others consume allocations from the output of regalloc2, and the only thing keeping them in sync is having all three places agree on the ordering.

So there's definitely one bug here in that `x64_get_operands` did the `reg_use` calls before `reg_def`, and `emit` called `allocs.next` in that same order, but `pretty_print` called `allocs.next` in a different order. And this PR fixes that bug, but then reveals that something doesn't line up, somewhere.

I have a commit locally to get rid of this ordering dependency from `pretty_print` and `emit`, so that only `get_operands` matters and it can use any order it wants, and we'll never have this problem again. But that commit changes the pretty-printing behavior in the same way that this PR does, by implicitly correcting the order in the pretty-printer. So I want to get this fixed and reviewed by itself first, so we can think about it without the rest of the implications of that change.

> * if this disagrees with the Capstone disassembly, shouldn't some test fail here?

As far as I know, there are no tests which compare our pretty-printer output with Capstone's output. Maybe there should be! It's tricky in cases like pseudo-instructions or synthetic address modes, but we could probably test a lot of instructions easily enough.

So the only way this shows up right now is that for precise-output compile filetests we list our pretty-printer output, and then we list Capstone's output, and a human has to decide whether those are close enough. In this PR, I used `CRANELIFT_TEST_BLESS` to update the one filetest which 
used this instruction, and saw that the pretty-printer output changed but the Capstone output didn't, and now I don't know what to do.

The actual binary machine code that's emitted isn't changing, so it's not surprising that none of the runtests failed, or that the Capstone output didn't change either.

Note that there are currently exactly two instructions which use our `Inst::XmmRmREvex` representation, [`vpmullq`](https://www.felixcloutier.com/x86/pmulld:pmullq) and [`vpsraq`](https://www.felixcloutier.com/x86/psraw:psrad:psraq). We don't currently have any filetests which contain `vpmullq`, and there's exactly one use of this non-immediate form of `vpsraq`. So I don't know whether both instructions are affected.

> * are we aiming for Intel syntax here or AT&T?

We're using roughly AT&T syntax in the pretty-printer output. There are good arguments for switching to Intel syntax, but Trevor tried doing that a while ago and found it's a lot of work. So instead we configure Capstone into AT&T mode to be consistent with our existing choices.

Before this PR, our pretty-printer was effectively printing `{src2}, {dst}, {src1}`. I don't think that makes sense for either Intel or AT&T syntax, does it?

In our sole filetest example, it happens that `dst` and `src1` get assigned the same register, because of ABI constraints I guess, so I suppose Capstone could be printing this in `{src2}, {src1}, {dst}` order and we wouldn't be able to tell the difference. Would that be reasonable for AT&T syntax?

I think one possibility is that Capstone's version of AT&T syntax has the operands backwards for this particular instruction. I wouldn't blame them if the non-Intel (and non-default) printing mode on a single random SIMD instruction wasn't checked carefully.
jameysharp: Based on our discussion in the Cranelift meeting today I've updated this PR, and everything makes sense to me now. Thank you so much to @abrown and @fitzgen for helping me figure this out! I've updated the commit message to explain what happened:

The operand collector had these operands in src1/src2/dst order, but the pretty-printer fetched the allocations in dst/src1/src2 order instead.

Although our pretty-printer looked like it was printing src1/src2/dst, because it consumed operands in the wrong order, what it actually printed was src2/dst/src1.

Meanwhile, Capstone actually uses src2/src1/dst order in AT&T mode. (GNU objdump agrees.)

In the only filetest covering the vpsraq instruction, our output agreed with Capstone because register allocation picked the same register for both src1 and dst, so the two orders were indistinguishable. I've extended the filetest to force register allocation to pick different registers.

This format is also used for vpmullq, but we didn't have any compile filetests covering that instruction, so I've added one with the same register allocation pattern.

Now our pretty-printer agrees with Capstone on both instructions.

Patch:
diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs
index ceb0d2962..5c1e84a09 100644
--- a/cranelift/codegen/src/isa/x64/inst/mod.rs
+++ b/cranelift/codegen/src/isa/x64/inst/mod.rs
@@ -1179,11 +1179,11 @@ impl PrettyPrint for Inst {
                 dst,
                 ..
             } => {
-                let dst = pretty_print_reg(dst.to_reg().to_reg(), 8, allocs);
                 let src1 = pretty_print_reg(src1.to_reg(), 8, allocs);
                 let src2 = src2.pretty_print(8, allocs);
+                let dst = pretty_print_reg(dst.to_reg().to_reg(), 8, allocs);
                 let op = ljustify(op.to_string());
-                format!("{op} {src1}, {src2}, {dst}")
+                format!("{op} {src2}, {src1}, {dst}")
             }
 
             Inst::XmmRmREvex3 {
@@ -1199,7 +1199,7 @@ impl PrettyPrint for Inst {
                 let src3 = src3.pretty_print(8, allocs);
                 let dst = pretty_print_reg(dst.to_reg().to_reg(), 8, allocs);
                 let op = ljustify(op.to_string());
-                format!("{op} {src1}, {src2}, {src3}, {dst}")
+                format!("{op} {src3}, {src2}, {src1}, {dst}")
             }
 
             Inst::XmmMinMaxSeq {
diff --git a/crates/environ/src/compile/module_environ.rs b/crates/environ/src/compile/module_environ.rs
index 67f53bdde..7582f9c02 100644
--- a/crates/environ/src/compile/module_environ.rs
+++ b/crates/environ/src/compile/module_environ.rs
@@ -494,19 +494,8 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
                             offset_expr,
                         } => {
                             let table_index = TableIndex::from_u32(table_index.unwrap_or(0));
-                            let mut offset_expr_reader = offset_expr.get_binary_reader();
-                            let (base, offset) = match offset_expr_reader.read_operator()? {
-                                Operator::I32Const { value } => (None, value.unsigned()),
-                                Operator::GlobalGet { global_index } => {
-                                    (Some(GlobalIndex::from_u32(global_index)), 0)
-                                }
-                                ref s => {
-                                    bail!(WasmError::Unsupported(format!(
-                                        "unsupported init expr in element section: {:?}",
-                                        s
-                                    )));
-                                }
-                            };
+                            let (offset, escaped) = ConstExpr::from_wasmparser(offset_expr)?;
+                            debug_assert!(escaped.is_empty());
 
                             self.result
                                 .module
@@ -514,7 +503,6 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
                                 .segments
                                 .push(TableSegment {
                                     table_index,
-                                    base,
                                     offset,
                                     elements: elements.into(),
                                 });
@@ -1165,14 +1153,15 @@ impl ModuleTranslation<'_> {
             // If the base of this segment is dynamic, then we can't
             // include it in the statically-built array of initial
             // contents.
-            if segment.base.is_some() {
-                break;
-            }
+            let offset = match segment.offset.ops() {
+                &[ConstOp::I32Const(offset)] => offset.unsigned(),
+                _ => break,
+            };
 
             // Get the end of this segment. If out-of-bounds, or too
             // large for our dense table representation, then skip the
             // segment.
-            let top = match segment.offset.checked_add(segment.elements.len()) {
+            let top = match offset.checked_add(segment.elements.len()) {
                 Some(top) => top,
                 None => break,
             };
@@ -1228,7 +1217,7 @@ impl ModuleTranslation<'_> {
             if precomputed.len() < top as usize {
                 precomputed.resize(top as usize, FuncIndex::reserved_value());
             }
-            let dst = &mut precomputed[(segment.offset as usize)..(top as usize)];
+            let dst = &mut precomputed[offset as usize..top as usize];
             dst.copy_from_slice(&function_elements);
 
             // advance the iterator to see the next segment
diff --git a/crates/environ/src/module.rs b/crates/environ/src/module.rs
index b7fe41653..7d3bede30 100644
--- a/crates/environ/src/module.rs
+++ b/crates/environ/src/module.rs
@@ -427,10 +427,8 @@ pub enum TableInitialValue {
 pub struct TableSegment {
     /// The index of a table to initialize.
     pub table_index: TableIndex,
-    /// Optionally, a global variable giving a base index.
-    pub base: Option<GlobalIndex>,
-    /// The offset to add to the base.
-    pub offset: u32,
+    /// The base offset to start this segment at.
+    pub offset: ConstExpr,
     /// The values to write into the table elements.
     pub elements: TableSegmentElements,
 }
diff --git a/crates/wasmtime/src/runtime/vm/instance/allocator.rs b/crates/wasmtime/src/runtime/vm/instance/allocator.rs
index fc7c11fa7..b5e701fc4 100644
--- a/crates/wasmtime/src/runtime/vm/instance/allocator.rs
+++ b/crates/wasmtime/src/runtime/vm/instance/allocator.rs
@@ -9,8 +9,8 @@ use anyhow::{anyhow, bail, Result};
 use std::{alloc, any::Any, mem, ptr, sync::Arc};
 use wasmtime_environ::{
     DefinedMemoryIndex, DefinedTableIndex, HostPtr, InitMemory, MemoryInitialization,
-    MemoryInitializer, MemoryPlan, Module, PrimaryMap, TableInitialValue, TablePlan, TableSegment,
-    Trap, VMOffsets, WasmHeapTopType, WasmValType, WASM_PAGE_SIZE,
+    MemoryInitializer, MemoryPlan, Module, PrimaryMap, TableInitialValue, TablePlan, Trap,
+    VMOffsets, WasmHeapTopType, WasmValType, WASM_PAGE_SIZE,
 };
 
 #[cfg(feature = "gc")]
@@ -525,24 +525,18 @@ pub trait InstanceAllocator: InstanceAllocatorImpl {
 // must use the defaults. This blanket impl provides both of those things.
 impl<T: InstanceAllocatorImpl> InstanceAllocator for T {}
 
-fn get_table_init_start(init: &TableSegment, instance: &mut Instance) -> Result<u32> {
-    match init.base {
-        Some(base) => {
-            let val = unsafe { *(*instance.defined_or_imported_global_ptr(base)).as_u32() };
-
-            init.offset
-                .checked_add(val)
-                .ok_or_else(|| anyhow!("element segment global base overflows"))
-        }
-        None => Ok(init.offset),
-    }
-}
-
 fn check_table_init_bounds(instance: &mut Instance, module: &Module) -> Result<()> {
+    let mut const_evaluator = ConstExprEvaluator::default();
+
     for segment in module.table_initialization.segments.iter() {
         let table = unsafe { &*instance.get_table(segment.table_index) };
-        let start = get_table_init_start(segment, instance)?;
-        let start = usize::try_from(start).unwrap();
+        let mut context = ConstEvalContext::new(instance, module);
+        let start = unsafe {
+            const_evaluator
+                .eval(&mut context, &segment.offset)
+                .expect("const expression should be valid")
+        };
+        let start = usize::try_from(start.get_u32()).unwrap();
         let end = start.checked_add(usize::try_from(segment.elements.len()).unwrap());
 
         match end {
@@ -610,12 +604,17 @@ fn initialize_tables(instance: &mut Instance, module: &Module) -> Result<()> {
     // iterates over all segments (Segments mode) or leftover
     // segments (FuncTable mode) to initialize.
     for segment in module.table_initialization.segments.iter() {
-        let start = get_table_init_start(segment, instance)?;
+        let mut context = ConstEvalContext::new(instance, module);
+        let start = unsafe {
+            const_evaluator
+                .eval(&mut context, &segment.offset)
+                .expect("const expression should be valid")
+        };
         instance.table_init_segment(
             &mut const_evaluator,
             segment.table_index,
             &segment.elements,
-            start,
+            start.get_u32(),
             0,
             segment.elements.len(),
         )?;


Test patch:
diff --git a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs
index 454e768c0..c55540ef4 100644
--- a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs
+++ b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs
@@ -61,6 +61,7 @@ impl Inst {
     }
 
     fn xmm_rm_r_evex(op: Avx512Opcode, src1: Reg, src2: RegMem, dst: Writable<Reg>) -> Self {
+        debug_assert_ne!(op, Avx512Opcode::Vpermi2b);
         src2.assert_regclass_is(RegClass::Float);
         debug_assert!(src1.class() == RegClass::Float);
         debug_assert!(dst.to_reg().class() == RegClass::Float);
@@ -72,6 +73,27 @@ impl Inst {
         }
     }
 
+    fn xmm_rm_r_evex3(
+        op: Avx512Opcode,
+        src1: Reg,
+        src2: Reg,
+        src3: RegMem,
+        dst: Writable<Reg>,
+    ) -> Self {
+        debug_assert_eq!(op, Avx512Opcode::Vpermi2b);
+        src3.assert_regclass_is(RegClass::Float);
+        debug_assert!(src1.class() == RegClass::Float);
+        debug_assert!(src2.class() == RegClass::Float);
+        debug_assert!(dst.to_reg().class() == RegClass::Float);
+        Inst::XmmRmREvex3 {
+            op,
+            src1: Xmm::new(src1).unwrap(),
+            src2: Xmm::new(src2).unwrap(),
+            src3: XmmMem::new(src3).unwrap(),
+            dst: WritableXmm::from_writable_reg(dst).unwrap(),
+        }
+    }
+
     // TODO Can be replaced by `Inst::move` (high-level) and `Inst::unary_rm_r` (low-level)
     fn xmm_mov(op: SseOpcode, src: RegMem, dst: Writable<Reg>) -> Inst {
         src.assert_regclass_is(RegClass::Float);
@@ -4189,19 +4211,37 @@ fn test_x64_emit() {
     insns.push((
         Inst::xmm_rm_r_evex(Avx512Opcode::Vpmullq, xmm10, RegMem::reg(xmm14), w_xmm1),
         "62D2AD0840CE",
-        "vpmullq %xmm10, %xmm14, %xmm1",
+        "vpmullq %xmm14, %xmm10, %xmm1",
     ));
 
     insns.push((
-        Inst::xmm_rm_r_evex(Avx512Opcode::Vpermi2b, xmm10, RegMem::reg(xmm14), w_xmm1),
+        Inst::xmm_rm_r_evex(Avx512Opcode::Vpsraq, xmm10, RegMem::reg(xmm14), w_xmm1),
+        "62D1AD08E2CE",
+        "vpsraq  %xmm14, %xmm10, %xmm1",
+    ));
+
+    insns.push((
+        Inst::xmm_rm_r_evex3(
+            Avx512Opcode::Vpermi2b,
+            xmm1,
+            xmm10,
+            RegMem::reg(xmm14),
+            w_xmm1,
+        ),
         "62D22D0875CE",
-        "vpermi2b %xmm10, %xmm14, %xmm1",
+        "vpermi2b %xmm14, %xmm10, %xmm1, %xmm1",
     ));
 
     insns.push((
-        Inst::xmm_rm_r_evex(Avx512Opcode::Vpermi2b, xmm0, RegMem::reg(xmm1), w_xmm2),
+        Inst::xmm_rm_r_evex3(
+            Avx512Opcode::Vpermi2b,
+            xmm2,
+            xmm0,
+            RegMem::reg(xmm1),
+            w_xmm2,
+        ),
         "62F27D0875D1",
-        "vpermi2b %xmm0, %xmm1, %xmm2",
+        "vpermi2b %xmm1, %xmm0, %xmm2, %xmm2",
     ));
 
     insns.push((
diff --git a/cranelift/filetests/filetests/isa/x64/shuffle-avx512.clif b/cranelift/filetests/filetests/isa/x64/shuffle-avx512.clif
index 2ccae2002..e0dad66c7 100644
--- a/cranelift/filetests/filetests/isa/x64/shuffle-avx512.clif
+++ b/cranelift/filetests/filetests/isa/x64/shuffle-avx512.clif
@@ -15,7 +15,7 @@ block0(v0: i8x16, v1: i8x16):
 ;   movdqa  %xmm0, %xmm5
 ;   movdqu  const(0), %xmm0
 ;   movdqa  %xmm5, %xmm6
-;   vpermi2b %xmm0, %xmm6, %xmm1, %xmm0
+;   vpermi2b %xmm1, %xmm6, %xmm0, %xmm0
 ;   movq    %rbp, %rsp
 ;   popq    %rbp
 ;   ret
@@ -54,7 +54,7 @@ block0(v0: i8x16, v1: i8x16):
 ;   movdqa  %xmm0, %xmm5
 ;   movdqu  const(0), %xmm0
 ;   movdqa  %xmm5, %xmm6
-;   vpermi2b %xmm0, %xmm6, %xmm1, %xmm0
+;   vpermi2b %xmm1, %xmm6, %xmm0, %xmm0
 ;   movq    %rbp, %rsp
 ;   popq    %rbp
 ;   ret
diff --git a/cranelift/filetests/filetests/isa/x64/simd-i64x2-mul-avx512.clif b/cranelift/filetests/filetests/isa/x64/simd-i64x2-mul-avx512.clif
new file mode 100644
index 000000000..6f0e6dbc8
--- /dev/null
+++ b/cranelift/filetests/filetests/isa/x64/simd-i64x2-mul-avx512.clif
@@ -0,0 +1,33 @@
+test compile precise-output
+target x86_64 sse42 has_avx has_avx2 has_avx512dq has_avx512vl
+
+function %imul(i64x2, i64x2) -> i64x2, i64x2 {
+block0(v0: i64x2, v1: i64x2):
+  ;; Force register allocation to pick a different destination than
+  ;; source for at least one of these instructions.
+  v2 = imul v0, v1
+  v3 = imul v2, v1
+  return v2, v3
+}
+
+; VCode:
+;   pushq   %rbp
+;   movq    %rsp, %rbp
+; block0:
+;   vpmullq %xmm1, %xmm0, %xmm0
+;   vpmullq %xmm1, %xmm0, %xmm1
+;   movq    %rbp, %rsp
+;   popq    %rbp
+;   ret
+;
+; Disassembled:
+; block0: ; offset 0x0
+;   pushq %rbp
+;   movq %rsp, %rbp
+; block1: ; offset 0x4
+;   vpmullq %xmm1, %xmm0, %xmm0
+;   vpmullq %xmm1, %xmm0, %xmm1
+;   movq %rbp, %rsp
+;   popq %rbp
+;   retq
+
diff --git a/cranelift/filetests/filetests/isa/x64/simd-i64x2-shift-avx512.clif b/cranelift/filetests/filetests/isa/x64/simd-i64x2-shift-avx512.clif
index 22210f479..d7a6796b9 100644
--- a/cranelift/filetests/filetests/isa/x64/simd-i64x2-shift-avx512.clif
+++ b/cranelift/filetests/filetests/isa/x64/simd-i64x2-shift-avx512.clif
@@ -1,19 +1,26 @@
 test compile precise-output
 target x86_64 sse42 has_avx has_avx2 has_avx512f has_avx512vl
 
-function %sshr(i64x2, i64) -> i64x2 {
+function %sshr(i64x2, i64) -> i64x2, i64x2 {
 block0(v0: i64x2, v1: i64):
+  ;; Force register allocation to pick a different destination than
+  ;; source for at least one of these instructions.
   v2 = sshr v0, v1
-  return v2
+  v3 = sshr v2, v1
+  return v2, v3
 }
 
 ; VCode:
 ;   pushq   %rbp
 ;   movq    %rsp, %rbp
 ; block0:
+;   movq    %rdi, %r9
+;   andq    %r9, $63, %r9
+;   vmovd   %r9d, %xmm1
+;   vpsraq  %xmm1, %xmm0, %xmm0
 ;   andq    %rdi, $63, %rdi
-;   vmovd   %edi, %xmm5
-;   vpsraq  %xmm5, %xmm0, %xmm0
+;   vmovd   %edi, %xmm1
+;   vpsraq  %xmm1, %xmm0, %xmm1
 ;   movq    %rbp, %rsp
 ;   popq    %rbp
 ;   ret
@@ -23,9 +30,13 @@ block0(v0: i64x2, v1: i64):
 ;   pushq %rbp
 ;   movq %rsp, %rbp
 ; block1: ; offset 0x4
+;   movq %rdi, %r9
+;   andq $0x3f, %r9
+;   vmovd %r9d, %xmm1
+;   vpsraq %xmm1, %xmm0, %xmm0
 ;   andq $0x3f, %rdi
-;   vmovd %edi, %xmm5
-;   vpsraq %xmm5, %xmm0, %xmm0
+;   vmovd %edi, %xmm1
+;   vpsraq %xmm1, %xmm0, %xmm1
 ;   movq %rbp, %rsp
 ;   popq %rbp
 ;   retq