Closed Bug 1337871 Opened 3 years ago Closed 3 years ago

Fix EmitBaselineEnterStubFrame and EmitBaselineLeaveStubFrame to not clobber the tail call register

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
This is really nice:

* We no longer have to use AutoStubFrame to reserve this register.

* This means Baseline CacheIR instructions that use stub frames get an extra scratch register, which we really need on x86. This is what prompted me to work on this because StoreDenseElement really wants an extra register.

* We can now implement EmitBaselineEnterStubFrame a bit more efficiently on x86 and x64 because we can eliminate an add instruction.

Note that this only affects x86 and x64: other platforms have a link register that stores the return address and this register is not allocatable.

This patch also removes EmitIonEnterStubFrame and EmitIonLeaveStubFrame, because we no longer have shared stubs that use stub frames.
Attachment #8835003 - Flags: review?(evilpies)
Summary: Fix EmitBaselineEnterStubFrame and EmitLeaveEnterStubFrame to not clobber the tail call register → Fix EmitBaselineEnterStubFrame and EmitBaselineLeaveStubFrame to not clobber the tail call register
Comment on attachment 8835003 [details] [diff] [review]
Patch

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

::: js/src/jit/x64/SharedICHelpers-x64.h
@@ +172,5 @@
> +    //   - BaselineStackReg
> +    //   - sizeof(return address)
> +    //
> +    // The two constants cancel each other out, so we can just calculate
> +    // BaselineStackReg - BaselineFrameReg.

This should be BaselineFrameReg - BaselineStackReg and same for x86. I fixed this locally.
Comment on attachment 8835003 [details] [diff] [review]
Patch

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

Wow an extra scratch register for free is super nice.
Attachment #8835003 - Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7000da4b6e2
Clean up and optimize code for entering/leaving Baseline stub frames. r=evilpie
Attached patch Remove more codeSplinter Review
We can also remove EmitIonCallVM and the JitFrame_IonStub frame type. This also cleans up some places where we passed ICStubEngine.

 15 files changed, 44 insertions(+), 148 deletions(-)
Attachment #8835636 - Flags: review?(hv1989)
Keywords: leave-open
I actually wanted to add engine to IRGenerator, so that we can log it with the CacheIRSpewer.
(In reply to Tom Schuster [:evilpie] from comment #5)
> I actually wanted to add engine to IRGenerator, so that we can log it with
> the CacheIRSpewer.

OK we can still do that.. Maybe pass it as first/second argument to the constructors.
Attachment #8835636 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af14b63b1368
part 2 - Remove some shared IC code that's no longer used. r=h4writer
Keywords: leave-open
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/af14b63b1368
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.