Closed Bug 1337060 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bbouvier, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached file i64-x86.js
Another baseline compiler compilation crash on x86 this time. See attached test case, not reduced yet.
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
        )
)
Hm yeah, I could see how that might be an interesting regalloc problem.
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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P1
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).
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.
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.
Attachment #8834362 - Flags: review?(bbouvier)
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.
Attachment #8834362 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/149af6fc70105502ce5ffeab56890751b5c91066
Bug 1337060 - wasm baseline, avoid register starvation for i64 select on x86. r=bbouvier
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...
It's my own fault for not expediting bug 1311294...
Also release tmp + test.
Attachment #8834437 - Flags: review?(lhansen)
Comment on attachment 8834437 [details] [diff] [review]
release-tmp.patch

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

Thanks!
Attachment #8834437 - Flags: review?(lhansen) → review+
Blocks: 1311294
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a357a52ffa6
Release tmp after using it in emitSelect on x86; r=lth
https://hg.mozilla.org/mozilla-central/rev/149af6fc7010
https://hg.mozilla.org/mozilla-central/rev/0a357a52ffa6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.