1337060 - Assertion failure: hasInt64(), at /code/mozilla-inbound/js/src/wasm/WasmBaselineCompile.cpp:722

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: i64-x86.js

Another baseline compiler compilation crash on x86 this time. See attached test case, not reduced yet.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Reduced to:
    
(module
        (func (export "func_7") (result i64) (param f64)
            i64.const 15642292384964472254
            i64.const 16488027933427574729
            i64.const 12438406169728556774
            i64.const 6282271200771070010
            i64.lt_s
            select
        )
)

Comment 2

[Lars T Hansen [:lth]]

Hm yeah, I could see how that might be an interesting regalloc problem.

Comment 3

[Lars T Hansen [:lth]]

The 'select' opcode for i64 is technically correct but keeps four values live in registers at the same time, and this is not possible on x86.  We should be able to defer the popping of the operands until after the branch.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Mid air collision, I was about to post:

We're starving the register allocator! Because of the latent i64.lt_s, there are 2 i64 registers (4 32-bits registers) needed for the comparison, but we pop the two select inputs (4 other 32 bits registers) before. x86 does have only 6 available registers after the scratch x86 register is taken in rabaldr.

I think a possible fix would be to pop the registers *after* performing the actual branch. This would generate more code, but this is necessary to ensure registers are available. There might be other alternatives, like not doing the efficient latent comparison (the result of the comparison gets into an i32, and we cjump wrt this i32 value).

Comment 5

[Lars T Hansen [:lth]]

Yes, popping after the branch is the easiest solution.  However the resulting control flow diamond is a little hairy because both branches manipulate the stack; the nice thing about popping before the branch is that there is no stack manipulation inside the diamond.  Still, clearly we can do this.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1337060-dont-starve.patch

Patch + TC.  It's hairy both to manipulate the stack differently along the two edges (because the baseline compiler does not have any meaningful machinery for saving and restoring its state) and to turn off the optimization for this particular case of select (because that means adding code for this several places in the compiler and complicating that machinery).

Instead I've made a point fix that uses the latent branch to set a flag in a temp and then branches on that flag subsequently.  The resulting code is very similar to what we would have gotten by turning off the optimization, but the complexity is local, not global.

There are open bugs on avoiding sync() before 'if' and 'block' opcodes, and on keeping locals in registers, and if they ever go anywhere we'll acquire the machinery to save and restore eval stack + regalloc state.  If so, we can revisit select.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8834362 [details] [diff] [review]
bug1337060-dont-starve.patch

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

Simple indeed, thanks for the patch.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +6417,5 @@
> +        // we normally pop before executing the branch.  On x86 this is one
> +        // value too many, so we need to generate more complicated code here, and
> +        // for simplicity's sake we do so even if the branch operands are not Int64.
> +        // However, the resulting diamond CF is complicated since the branches
> +        // of the diamond would have to stay synchronized wrt their stack state

nit: please explicit wrt.

Comment 8

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/149af6fc70105502ce5ffeab56890751b5c91066
Bug 1337060 - wasm baseline, avoid register starvation for i64 select on x86. r=bbouvier

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Another instance of this bug, even with this patch:

(module
        (func (export "func_1") (param f32)
            ;; Top level expr.
            i64.const 8945138605783157125
            i64.const 6888872119727767844
            i64.const 4339981856902867500
            i32.const 4242157984
            select
            i32.const 1313569729
            select
            ;; Top level expr.
            i64.const 3301395488065409639
            i64.const 8323206560143737710
            i32.const 2535109750
            select
            drop drop
        )
    )

It seems that some i64 registers are never being released...

Comment 10

[Lars T Hansen [:lth]]

It's my own fault for not expediting bug 1311294...

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: release-tmp.patch

Also release tmp + test.

Comment 12

[Lars T Hansen [:lth]]

Comment on attachment 8834437 [details] [diff] [review]
release-tmp.patch

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

Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a357a52ffa6
Release tmp after using it in emitSelect on x86; r=lth

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/149af6fc7010
https://hg.mozilla.org/mozilla-central/rev/0a357a52ffa6