Last Comment Bug 701961 - IonMonkey: Compile JSOP_THIS
: IonMonkey: Compile JSOP_THIS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 713855 (view as bug list)
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-11 17:57 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-01-04 01:20 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JSOP_THIS (977 bytes, patch)
2011-12-26 17:10 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_THIS (3.69 KB, patch)
2012-01-03 11:33 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 17:57:20 PST
Necessary for benchmarks.
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-26 16:08:49 PST
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] 2011-12-26 17:10:58 PST
Created attachment 584358 [details] [diff] [review]
Implement JSOP_THIS
Comment 3 David Anderson [:dvander] 2011-12-27 12:09:52 PST
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] 2011-12-27 12:13:18 PST
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] 2012-01-03 11:33:51 PST
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.
Comment 6 David Anderson [:dvander] 2012-01-03 12:10:42 PST
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] 2012-01-03 17:26:39 PST
(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] 2012-01-03 19:01:19 PST
https://hg.mozilla.org/projects/ionmonkey/rev/b07c7276e785 (forgot reviewer, sorry)
Comment 9 Jan de Mooij [:jandem] 2012-01-04 01:20:52 PST
*** Bug 713855 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.