IonMonkey: Compile JSOP_THIS

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Necessary for benchmarks.
(Assignee)

Comment 1

6 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

6 years ago
Created attachment 584358 [details] [diff] [review]
Implement JSOP_THIS
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

6 years ago
Created attachment 585484 [details] [diff] [review]
Implement JSOP_THIS

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

6 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

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/b07c7276e785 (forgot reviewer, sorry)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Duplicate of this bug: 713855
You need to log in before you can comment on or make changes to this bug.