Closed Bug 1337060 Opened 3 years ago Closed 3 years ago
Assertion failure: has
Int64(), at /code/mozilla-inbound/js/src/wasm/Wasm Baseline Compile .cpp:722
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
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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a357a52ffa6 Release tmp after using it in emitSelect on x86; r=lth
You need to log in before you can comment on or make changes to this bug.