Closed Bug 1352506 Opened 5 years ago Closed 5 years ago

Ion: Automatically call gen->setPerformsCall() if a LIR instruction performs a call

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(3 files, 3 obsolete files)

No description provided.
Priority: -- → P3
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...
Attached patch setperformscall.patch (obsolete) — Splinter Review
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
Attached patch setperformscall.patch (obsolete) — Splinter Review
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
Attached patch dontforcebasline.patch (obsolete) — Splinter Review
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)
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 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 on attachment 8858335 [details] [diff] [review]
dontforcebasline.patch

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

Hah, right
Attachment #8858335 - Flags: review?(luke) → review+
(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.
Comment on attachment 8858335 [details] [diff] [review]
dontforcebasline.patch

Moved and landed in bug 1359810
Attachment #8858335 - Attachment is obsolete: true
Duplicate of this bug: 1357048
Attached patch dead-code.patchSplinter Review
carrying forward r+'d patch from the dup'd bug
Attachment #8862414 - Flags: review+
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)
Attached patch 3.split.patchSplinter Review
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 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 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+
(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.
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
You need to log in before you can comment on or make changes to this bug.