1310158 - Wasm baseline regalloc failure on MacOS x86 with Clang-3.9

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

Consider this self-contained test case:

let bin = wasmTextToBinary(`
 (module (func (export "i64.test") (result i64)
	  (return (i64.const 0x0CABBA6E0ba66a6e))))`);
let mod = new WebAssembly.Module(bin);

The baseline compiler generates the following code using gcc 5.2.1 on Linux (where the jump is a jump to code that cleans up the stack and returns):

  [Codegen] movl       $0xba66a6e, %eax
  [Codegen] movl       $0xcabba6e, %edi
  [Codegen] jmp        .Lfrom75

It generates the following code using Clang-3.9 on Mac OS X:

  [Codegen] movl       $0xba66a6e, %ecx
  [Codegen] movl       $0xcabba6e, %eax
  [Codegen] movl       %ecx, %eax
  [Codegen] movl       %eax, %edi
  [Codegen] jmp        .Lfrom79

The second snipped is clearly wrong since eax is overwritten before its value can be obtained; this is a straightforward bug in how the return register is handled.

The real mystery is why different code is being generated on the two platforms.  The reason is that on 32-bit systems, the allocator for 64-bit registers (ie a pair) looks like this:

    Register64 allocInt64() {
        return Register64(availGPR_.takeAny(), availGPR_.takeAny());
    }

and the order of those calls is implementation-dependent.  That should be fixed; we want determinism when we can have it.

So, two fixes here: one for the regalloc bug, and one to curb the nondeterminism.

Comment 1

[Lars T Hansen [:lth]]

I should note that EDI:EAX is the 64-bit return register pair, of course.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1310158-determinism.patch

Make register allocation deterministic again.

This patch makes the test case fail on all platforms.  The patch can't land without its companion that fixes the regalloc bug.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1310158-return-value.patch

Clean up the handling of return values, both when returning them and when capturing them.  This code was probably correct up until 64-bit on 32-bit, but with register pairs it was becoming shaky.  This is better.

Comment 4

[Lars T Hansen [:lth]]

Try build is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22f590c45eb6

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8801090 [details] [diff] [review]
bug1310158-return-value.patch

Review of attachment 8801090 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +1492,5 @@
>          return r;
>      }
>  
> +    // Note, the stack top can be in one half of "specific" on 32-bit
> +    // systems.  We can optimize, but for simplicity, if the register

s/  / /

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8801078 [details] [diff] [review]
bug1310158-determinism.patch

Review of attachment 8801078 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +698,5 @@
>          MOZ_ASSERT(hasInt64());
>  #ifdef JS_PUNBOX64
>          return Register64(availGPR_.takeAny());
>  #else
> +        // Determinism, please.

Please remove comment.

Comment 7

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39eb040a44e6032ff65c2b367b7a57960a6192b1
Bug 1310158 - remove compiler dependency. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/90a743486346d4372d12233d27e5268c91c5cf82
Bug 1310158 - clean up return value handling. r=h4writer

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39eb040a44e6
https://hg.mozilla.org/mozilla-central/rev/90a743486346