Closed
Bug 1352506
Opened 7 years ago
Closed 7 years ago
Ion: Automatically call gen->setPerformsCall() if a LIR instruction performs a call
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(3 files, 3 obsolete files)
2.64 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
25.12 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
11.80 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
I just found out about mir->possiblyCalls(), which already has automation to call setPerformsCall(): http://searchfox.org/mozilla-central/source/js/src/jit/Lowering.cpp#4903-4904 This solves a few cases, but there are other which are less trivial, including a few ARM callouts (soft div/mod) which don't use the LCallInstructionHelper infra at all. What's going on there? It seems we're trying to be smart and avoid some register spilling before the call site...
Assignee | ||
Comment 2•7 years ago
|
||
Not setting r? yet: - I'd like to wait for the next train cycle before getting this landed. - try build first https://treeherder.mozilla.org/#/jobs?repo=try&revision=f800e8ba83d9eed5538242b37538b4335a6d238a
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
Bad news, everyone: there was a bug in the lowering of soft div/mod. Good news, everyone: the ARM callouts paths for softdiv/mod are effectively tested on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f800e8ba83d9eed5538242b37538b4335a6d238a Retried locally, fixes were trivial.
Attachment #8858321 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
It's silly: with --wasm-always-baseline, we don't even consider if the platform supports baseline compilation. So on ARM without IDIV, we try to use idiv and this generates an illegal instruction. The previous comment shows that the soft div path is effectively tested on treeherder (jsreftests, so no wasm). Maybe we should have wasm tests in jsreftests too...
Attachment #8858335 -
Flags: review?(luke)
Comment 5•7 years ago
|
||
setPerformsCall is used to ensure we emit an overrecursion check right? Do we really need one for things like div/mod helper calls? I think our stack limits have a buffer that should be big enough to allow simple calls like that.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > setPerformsCall is used to ensure we emit an overrecursion check right? Do > we really need one for things like div/mod helper calls? I think our stack > limits have a buffer that should be big enough to allow simple calls like > that. In WebAssembly, it also pre-ensures statically known stack alignment in codegen, so that we can use aligned ABI calls everywhere: http://searchfox.org/mozilla-central/source/js/src/jit/shared/CodeGenerator-shared.cpp#99-102
Comment 7•7 years ago
|
||
Comment on attachment 8858335 [details] [diff] [review] dontforcebasline.patch Review of attachment 8858335 [details] [diff] [review]: ----------------------------------------------------------------- Hah, right
Attachment #8858335 -
Flags: review?(luke) → review+
Comment 8•7 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #6) > In WebAssembly, it also pre-ensures statically known stack alignment in > codegen, so that we can use aligned ABI calls everywhere: > http://searchfox.org/mozilla-central/source/js/src/jit/shared/CodeGenerator- > shared.cpp#99-102 Ah I see. I don't know if we should conflate the two - we'll now insert more overrecursion checks than before. It probably doesn't matter too much but worth keeping in mind.
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8858335 [details] [diff] [review] dontforcebasline.patch Moved and landed in bug 1359810
Attachment #8858335 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
carrying forward r+'d patch from the dup'd bug
Attachment #8862414 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
Merged the two patches from this bug and the dup'd one together for a better outcome. I think it's ready for review now.
Attachment #8858332 -
Attachment is obsolete: true
Attachment #8862416 -
Flags: review?(sunfish)
Assignee | ||
Comment 13•7 years ago
|
||
There really aren't that many uses of setPerformsCall() and performsCall(), so it's easy to just disentangle the two meanings.
Attachment #8862419 -
Flags: review?(jdemooij)
Comment 14•7 years ago
|
||
Comment on attachment 8862419 [details] [diff] [review] 3.split.patch Review of attachment 8862419 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit/shared/Lowering-shared-inl.h @@ +234,5 @@ > { > lir->setMir(mir); > > MOZ_ASSERT(lir->isCall()); > + gen->setNeedsStaticStackAlignment(); I was wondering why we no longer need the overrecursion check here for things like LCallGeneric, but I guess the possiblyCalls() check takes care of that?
Attachment #8862419 -
Flags: review?(jdemooij) → review+
Comment 15•7 years ago
|
||
Comment on attachment 8862416 [details] [diff] [review] setperformscall.patch Review of attachment 8862416 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #8862416 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] (PTO until May 8) from comment #14) > ::: js/src/jit/shared/Lowering-shared-inl.h > @@ +234,5 @@ > > { > > lir->setMir(mir); > > > > MOZ_ASSERT(lir->isCall()); > > + gen->setNeedsStaticStackAlignment(); > > I was wondering why we no longer need the overrecursion check here for > things like LCallGeneric, but I guess the possiblyCalls() check takes care > of that? Yes indeed. Patch 2 of this bug is the one that actually introduced the gen->setPerformsCall() here. The possiblyCalls() behavior predates this bug and remains effective.
Comment 17•7 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b592b4bf8629 Remove dead code in the backtracking allocator; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb66cc5c1d9 Automatically setPerformsCall for LIR call instructions; r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/082674f71ff2 Don't conflate need for overrecursed check and static alignment in MIRGenerator; r=jandem
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b592b4bf8629 https://hg.mozilla.org/mozilla-central/rev/9eb66cc5c1d9 https://hg.mozilla.org/mozilla-central/rev/082674f71ff2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•