Closed
Bug 1337060
Opened 8 years ago
Closed 8 years ago
Assertion failure: hasInt64(), at /code/mozilla-inbound/js/src/wasm/WasmBaselineCompile.cpp:722
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bbouvier, Assigned: lth)
References
Details
Attachments
(3 files)
(deleted),
application/javascript
|
Details | |
(deleted),
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
Another baseline compiler compilation crash on x86 this time. See attached test case, not reduced yet.
Reporter | ||
Comment 1•8 years ago
|
||
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
)
)
Assignee | ||
Comment 2•8 years ago
|
||
Hm yeah, I could see how that might be an interesting regalloc problem.
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Reporter | ||
Comment 4•8 years ago
|
||
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).
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/149af6fc70105502ce5ffeab56890751b5c91066
Bug 1337060 - wasm baseline, avoid register starvation for i64 select on x86. r=bbouvier
Reporter | ||
Comment 9•8 years ago
|
||
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...
Assignee | ||
Comment 10•8 years ago
|
||
It's my own fault for not expediting bug 1311294...
Reporter | ||
Comment 11•8 years ago
|
||
Also release tmp + test.
Attachment #8834437 -
Flags: review?(lhansen)
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8834437 [details] [diff] [review]
release-tmp.patch
Review of attachment 8834437 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8834437 -
Flags: review?(lhansen) → review+
Comment 13•8 years ago
|
||
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
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/149af6fc7010
https://hg.mozilla.org/mozilla-central/rev/0a357a52ffa6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•