Register allocation panic with simd comparison

[alexcrichton]

A fuzz bug came in over the weekend and bisection points to https://github.com/bytecodealliance/wasmtime/pull/3886 as the cause. This wasm module:

(module
  (func (result v128)
    global.get 0
    global.get 0
    i8x16.gt_u
  )
  (global (mut v128) v128.const i64x2 0 0)
)

fails with:

$ cargo run -q compile testcase0.wat
 ERROR cranelift_codegen::machinst::compile > Register allocation error for vcode
VCode_ShowWithRRU {{
  Entry block: 0
Block 0:
  (original IR block: block0)
  (successor: Block 1)
  (instruction range: 0 .. 12)
  Inst 0:   movq    %rdi, %v0J
  Inst 1:   movq    %rsi, %v1J
  Inst 2:   movups  64(%v0J), %v2V
  Inst 3:   movdqa  %v2V, %v7V
  Inst 4:   pmaxub  64(%v0J), %v7V
  Inst 5:   movdqa  %v7V, %v8V
  Inst 6:   pcmpeqb %v3V, %v8V
  Inst 7:   pcmpeqd %v9V, %v9V
  Inst 8:   movdqa  %v8V, %v4V
  Inst 9:   pxor    %v9V, %v4V
  Inst 10:   movdqa  %v4V, %v5V
  Inst 11:   jmp     label1
Block 1:
  (original IR block: block1)
  (instruction range: 12 .. 15)
  Inst 12:   movdqa  %v5V, %v6V
  Inst 13:   movdqa  %v6V, %xmm0
  Inst 14:   ret
}}

Error: Analysis(EntryLiveinValues([v3V]))
thread 'main' panicked at 'register allocation: Analysis(EntryLiveinValues([v3V]))', cranelift/codegen/src/machinst/compile.rs:98:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The non-minimized reproduction is here

[abrown]

I'll take a look...

[abrown]

The following ISLE code is doing the lowering:

https://github.com/bytecodealliance/wasmtime/blob/926d37589c60078f1749ef5735ee382b79187389/cranelift/codegen/src/isa/x64/lower.isle#L1472-L1476

In the snippet above, it looks like max is assigned v7V and b should be assigned the value coming from a load (i.e., v3V). But that load has been coalesced into the first operand of pmaxub (i.e.,64(%v0J)). @fitzgen, @alexcrichton: I can't remember how to avoid this load coalescing, though...

[abrown]

Correction: I can turn off load coalescing manually with the following, which fixes the issue:

diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs
index 3b9341420..6cc265be2 100644
--- a/cranelift/codegen/src/isa/x64/lower.rs
+++ b/cranelift/codegen/src/isa/x64/lower.rs
@@ -137,6 +137,10 @@ fn is_mergeable_load<C: LowerCtx<I = Inst>>(
         return None;
     }

+    if load_ty.is_vector() {
+        return None;
+    }
+
     // SIMD instructions can only be load-coalesced when the loaded value comes
     // from an aligned address.
     if load_ty.is_vector() && !insn_data.memflags().map_or(false, |f| f.aligned()) {

But that is not the right approach. What I meant is that there may have been a way to prevent this kind of thing from happening when an operand was resused but I can't remember exactly how Chris would do it.

[cfallin]

I'm not really here (out of office and vanishing after this comment!) but this is the same issue as 3934 I think. Compares need explicit put_in_reg, not implicit reg or mem.

[cfallin]

(sorry for not catching that in review, it's subtle!)

[abrown]

Thanks @cfallin! That's exactly what I couldn't remember!