Closed
Bug 867471
Opened 12 years ago
Closed 12 years ago
IonMonkey: Support JSOP_REST
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(3 files, 2 obsolete files)
18.54 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
5.41 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
10.24 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
Doesn't seem too bad, would be useful for some PJS stuff we're writing.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: general → shu
Assignee | ||
Comment 2•12 years ago
|
||
Remove stale comment
Attachment #743979 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #744988 -
Flags: review?(kvijayan)
Assignee | ||
Updated•12 years ago
|
Attachment #743980 -
Flags: review?(kvijayan)
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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•12 years ago
|
||
Thanks for the helpful and detailed review! Applied your fixes.
Attachment #743980 -
Attachment is obsolete: true
Attachment #745352 -
Flags: review?(kvijayan)
Comment 7•12 years ago
|
||
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•12 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 | ||
Comment 9•12 years ago
|
||
Parallel mode code for JSOP_REST. Depends on the refactoring in bug 869313.
Attachment #747808 -
Flags: review?(nmatsakis)
Updated•12 years ago
|
Attachment #747808 -
Flags: review?(nmatsakis) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ddd20f8bcb1c
https://hg.mozilla.org/mozilla-central/rev/b2216a10f95b
https://hg.mozilla.org/mozilla-central/rev/2f7967db9d25
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•