IonMonkey: Support JSOP_REST

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

6 years ago
Doesn't seem too bad, would be useful for some PJS stuff we're writing.
Assignee

Comment 1

6 years ago
Posted patch Part 1: Baseline (obsolete) — Splinter Review
Assignee: general → shu
Assignee

Comment 2

6 years ago
Posted patch Part 1: Baseline (obsolete) — Splinter Review
Remove stale comment
Attachment #743979 - Attachment is obsolete: true
Assignee

Comment 3

6 years ago
Attachment #744988 - Flags: review?(kvijayan)
Assignee

Updated

6 years ago
Attachment #743980 - Flags: review?(kvijayan)
Comment on attachment 743980 [details] [diff] [review]
Part 1: Baseline

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

::: js/src/ion/BaselineCompiler.cpp
@@ +2480,5 @@
> +    masm.movePtr(ImmGCPtr(type), R1.scratchReg());
> +
> +    ICRest_Fallback::Compiler stubCompiler(cx, numFormals);
> +    if (!emitOpIC(stubCompiler.getStub(&stubSpace_)))
> +        return false;

Most of this work is unnecessary in the main jitcode, and it increases the size of baseline jitcode.  Everything computed here that depends on the baseline frame can be put off for later.

frame.syncStack(0)
masm.movePtr(ImmGCPtr(type), R0.scratchReg());
ICRest_Fallback:Compiler stubCompiler(...);
...

It's better to just pass the type unconditionally to the IC chain.  If it's not needed, the IC chain can ignore it.  The branching code to load it conditionally will be more expensive than loading it unconditionally.

::: js/src/ion/BaselineIC.cpp
@@ +7556,5 @@
> +    masm.push(R1.scratchReg()); // type
> +    masm.push(R0.scratchReg()); // length
> +    masm.push(BaselineStubReg); // stub
> +
> +    return tailCallVM(DoCreateRestParameterInfo, masm);

As mentioned in the comment on BaselineIC.h, this code shouldn't hardcode numFormals_ in its generation.

That's not a big problem, though.  If you change DoCreateRestParameter to have the signature:

bool (*)(JSContext *, BaselineFrame *, ICRest_Fallback *, HandleTypeObject, MutableHandleValue)

Then instead of computing argument vp pointer here and passing it in, the C++ function can do that itself using the BaselineFrame pointer.  All you'd need in the stub entry code would be:

EmitRestoreTailCallReg(masm);
masm.push(R0.scratchReg()); // type
masm.push(BaselineStubReg); // stub
masm.pushBaselineFramePtr(BaselineFrameReg, R0.scratchReg()); // Baseline frame
return tailCallVM(...);

::: js/src/ion/BaselineIC.h
@@ +5299,5 @@
> +
> +    class Compiler : public ICStubCompiler {
> +      protected:
> +        uint32_t numFormals_;
> +        bool generateStubCode(MacroAssembler &masm);

You can't generate IC stub code that hardcodes numFormals_, unless you override the |getKey| method on the compiler to include the numFormals_ in the key (this way, a new ICRest_Fallback stubcode will get compiled for each value of numFormals).  However, that's not advisable because we can expect some high amount of variation in numFormals_, so it'll lead to many different versions of the stubcode being generated.

It's better to store the numFormals_ value on the stub itself (there's a convenient |uint16_t extra_| field that exists on all stubs for use in situations like this), and make the stubcode generic on that value.  See other ICStubs (e.g. TypeCheckPrimitiveSetStub) for examples of how extra_ can be used.
Attachment #743980 - Flags: review?(kvijayan)
Comment on attachment 744988 [details] [diff] [review]
Part 2: Sequential Ion

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

::: js/src/ion/Lowering.cpp
@@ +2454,5 @@
> +    LRest *lir = new LRest(useFixed(ins->numActuals(), CallTempReg0),
> +                           tempFixed(CallTempReg1),
> +                           tempFixed(CallTempReg2),
> +                           tempFixed(CallTempReg3));
> +    return defineReturn(lir, ins) && assignSafepoint(lir, ins);

I don't think this safePoint is needed.  Constructing a new array, or initializing it using the actual arguments, should both be repeatable.

::: js/src/ion/VMFunctions.cpp
@@ +697,5 @@
> +                  HandleObject res)
> +{
> +    JS_ASSERT_IF(res, res->isArray());
> +    JS_ASSERT_IF(res, !res->getDenseInitializedLength());
> +    JS_ASSERT_IF(res, res->type() == type);

Nit: These ASSERTS can be moved inside the if (res) { ... } block below, and made non-conditional.
Attachment #744988 - Flags: review?(kvijayan) → review+
Assignee

Comment 6

6 years ago
Thanks for the helpful and detailed review! Applied your fixes.
Attachment #743980 - Attachment is obsolete: true
Attachment #745352 - Flags: review?(kvijayan)
Comment on attachment 745352 [details] [diff] [review]
Part 1: Baseline

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

Nice.
Attachment #745352 - Flags: review?(kvijayan) → review+
Assignee

Comment 8

6 years ago
(In reply to Kannan Vijayan [:djvj] from comment #5)
> Comment on attachment 744988 [details] [diff] [review]
> Part 2: Sequential Ion
> 
> Review of attachment 744988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/Lowering.cpp
> @@ +2454,5 @@
> > +    LRest *lir = new LRest(useFixed(ins->numActuals(), CallTempReg0),
> > +                           tempFixed(CallTempReg1),
> > +                           tempFixed(CallTempReg2),
> > +                           tempFixed(CallTempReg3));
> > +    return defineReturn(lir, ins) && assignSafepoint(lir, ins);
> 
> I don't think this safePoint is needed.  Constructing a new array, or
> initializing it using the actual arguments, should both be repeatable.

Anything that uses callVM needs assignSafepoint, actually.
Assignee

Updated

6 years ago
Depends on: 869313
Assignee

Comment 9

6 years ago
Parallel mode code for JSOP_REST. Depends on the refactoring in bug 869313.
Attachment #747808 - Flags: review?(nmatsakis)
Attachment #747808 - Flags: review?(nmatsakis) → review+
Depends on: 875748
Depends on: 875777
You need to log in before you can comment on or make changes to this bug.