1768648 - MCompare::Compare_RefOrNull codegen could be better

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[André Bargull [:anba]]

var f = wasmEvalText(`
(module
  (func (param externref) (result i32)
    i32.const 1
    local.get 0
    ref.is_null
    br_if 0
    drop
    i32.const 0
  )
  (export "" (func 0))
)
`).exports[""];

print(disnative(f));

Prints:

;; prologue elided

00000024  48 33 c0                  xor %rax, %rax
00000027  3b f8                     cmp %eax, %edi
00000029  0f 94 c1                  setz %cl
0000002C  0f b6 c9                  movzx %cl, %ecx
0000002F  85 c9                     test %ecx, %ecx
00000031  0f 84 0a 00 00 00         jz 0x0000000000000041
00000037  b8 01 00 00 00            mov $0x01, %eax
0000003C  e9 02 00 00 00            jmp 0x0000000000000043
00000041  33 c0                     xor %eax, %eax
00000043  5d                        pop %rbp
00000044  c3                        ret

Two observations:

1. Lowering::visitTest() doesn't handle MCompare::Compare_RefOrNull. (https://hg.mozilla.org/try/rev/1453aee66aa82c44efb4c49671873357cff1f404)
2. RefOrNull types are compared as int32 values instead of pointers. (https://hg.mozilla.org/try/rev/f5c925b695817bfae828a4941ed6018804f36ad6)

Applying the two patches gives this codegen, which seems better resp. more correct:

;; prologue elided

00000024  48 33 c0                  xor %rax, %rax
00000027  48 3b f8                  cmp %rax, %rdi
0000002A  0f 85 0a 00 00 00         jnz 0x000000000000003A
00000030  b8 01 00 00 00            mov $0x01, %eax
00000035  e9 02 00 00 00            jmp 0x000000000000003C
0000003A  33 c0                     xor %eax, %eax
0000003C  5d                        pop %rbp
0000003D  c3                        ret

Or am I missing something here?

Comment 1

[Ryan Hunt [:rhunt]]

This seems likely to me. We've had i64 codegen quality issues from no extending lowering appropriately too. This is technically a part of ref-types, but I'm marking it as blocking wasm-gc for now.

Comment 2

[Ryan Hunt [:rhunt]]

Are you interested in putting the attached patches up for review? They look correct to me. If not, would we be able to take them?

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1768648 - Part 1: Compare Compare_RefOrNull as pointers. r=rhunt!

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1768648 - Part 2: Use LCompareAndBranch for Compare_RefOrNull. r=rhunt!

Depends on D152631

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f89d196c2b59
Part 1: Compare Compare_RefOrNull as pointers. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/fd53e107256c
Part 2: Use LCompareAndBranch for Compare_RefOrNull. r=rhunt

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f89d196c2b59
https://hg.mozilla.org/mozilla-central/rev/fd53e107256c