701961 - IonMonkey: Compile JSOP_THIS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Necessary for benchmarks.

Comment 1

[Nicolas B. Pierron [:nbp]]

This is currently the most needed opcode reported by abort messages in v8 & sunspider test suite when they are executed with '--ion -n' flags.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_THIS

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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).

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 7

[Nicolas B. Pierron [:nbp]]

(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()".

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/b07c7276e785 (forgot reviewer, sorry)