Closed
Bug 701961
Opened 13 years ago
Closed 13 years ago
IonMonkey: Compile JSOP_THIS
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.69 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Necessary for benchmarks.
Assignee | ||
Comment 1•13 years ago
|
||
This is currently the most needed opcode reported by abort messages in v8 & sunspider test suite when they are executed with '--ion -n' flags.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #584358 -
Flags: review?(dvander)
Comment on attachment 584358 [details] [diff] [review] Implement JSOP_THIS Review of attachment 584358 [details] [diff] [review]: ----------------------------------------------------------------- JSOP_THIS is unfortunately more complicated - see ComputeThis(). The logic is something like: * If |this| is an object, return |this|. * If |this| is null or undefined, |this| is globalObj->thisObject() * If |this| is a primitive, return js_PrimitiveToObject(this) So any time |this| is used where we don't already know that |this| has been computed, we need to replace it with a new SSA name. In this case it's okay to use the MIR node for |this| to determine whether |this| is already computed.
Attachment #584358 -
Flags: review?(dvander)
One option is to have a ComputeThis(Value) -> Value instruction that has a guard, and an out-of-line path for returning the new |this|. With TypeInference we can also determine the type ComputeThis will return (however there wouldn't be a type barrier, so we'd have to manually unbox).
Assignee | ||
Comment 5•13 years ago
|
||
Specialize this with type inference and compile JSOP_THIS only if the type is an object. Otherwise, abort the compilation with a message.
Attachment #584358 -
Attachment is obsolete: true
Attachment #585484 -
Flags: review?(dvander)
Comment on attachment 585484 [details] [diff] [review] Implement JSOP_THIS Review of attachment 585484 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +362,5 @@ > // -- ResumePoint(v0) > // > // As usual, it would be invalid for v1 to be captured in the initial > // resume point, rather than v0. > + current->add(actual); Whoops, good catch. @@ +3004,5 @@ > +{ > + // initParameters only initialized "this" after the following check, make > + // sure we can safely access thisSlot. > + if (!info().fun()) > + return false; Should this be an abort? Or is this an error? (As written it'll be OOM) @@ +3011,5 @@ > + MDefinition *thisParam = current->getSlot(info().thisSlot()); > + > + if (thisParam->type() != MIRType_Object) { > + IonSpew(IonSpew_Abort, "Cannot compile this, not an object."); > + return false; Instead: return abort("... otherwise this will act as OOM.
Attachment #585484 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to David Anderson [:dvander] from comment #6) > @@ +3004,5 @@ > > +{ > > + // initParameters only initialized "this" after the following check, make > > + // sure we can safely access thisSlot. > > + if (!info().fun()) > > + return false; > > Should this be an abort? Or is this an error? (As written it'll be OOM) I think this should be an assertion, because the bytecode is badly produced. So I just removed the check as the assertion is already done by "info().thisSlot()".
Assignee | ||
Comment 8•13 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/b07c7276e785 (forgot reviewer, sorry)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•