Closed Bug 867471 Opened 8 years ago Closed 8 years ago

IonMonkey: Support JSOP_REST

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(3 files, 2 obsolete files)

Doesn't seem too bad, would be useful for some PJS stuff we're writing.
Attached patch Part 1: Baseline (obsolete) — Splinter Review
Assignee: general → shu
Attached patch Part 1: Baseline (obsolete) — Splinter Review
Remove stale comment
Attachment #743979 - Attachment is obsolete: true
Attachment #744988 - Flags: review?(kvijayan)
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+
Attached patch Part 1: BaselineSplinter Review
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+
(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.
Depends on: 869313
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.