851913 - IonMonkey: support arrow functions

Status: RESOLVED DUPLICATE of bug 988993

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

In bug 848062, I couldn't immediately make Ion emit correct code for a script containing an arrow function. This followup bug is to fix that.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 726350 [details] [diff] [review]
v1

Review of attachment 726350 [details] [diff] [review]:
-----------------------------------------------------------------

That's the way to go, still some details to to make it cleaner.

::: js/src/ion/IonBuilder.cpp
@@ +6942,5 @@
> +        if (type != JSVAL_TYPE_OBJECT && type != JSVAL_TYPE_NULL && type != JSVAL_TYPE_UNDEFINED)
> +            return abort("Arrow function in script with maybe-primitive 'this'");
> +
> +        MInstruction *thisValue = MBox::New(current->getSlot(info().thisSlot()));
> +        current->add(thisValue);

You can remove the MBox::New and follow the suggestion made in (1), i-e:

MDefinition *thisValue = current->getSlot(info().thisSlot());
ins = MBoundLambda::New(current->scopeChain(), fun, thisValue);

You don't need to do a current->add as you are not creating any new instruction.

::: js/src/ion/IonBuilder.h
@@ +294,5 @@
>  
>      MDefinition *createThisScripted(MDefinition *callee);
>      MDefinition *createThisScriptedSingleton(HandleFunction target, MDefinition *callee);
>      MDefinition *createThis(HandleFunction target, MDefinition *callee);
> +    MDefinition *getThis();

This function is never defined.

::: js/src/ion/MIR.h
@@ +3668,5 @@
>  };
>  
> +class MBoundLambda
> +  : public MBinaryInstruction,
> +    public SingleObjectPolicy

(1) Having a Binary Instruction with a SingleObjectPolicy is a bit weird.  I usually prefer when the type policy match the number of operands, such as:

  : public MBinaryInstruction,
    public MixPolicy< ObjectPolicy<0>, BoxPolicy<1> >

In addition, this will remove the fact that we are manually boxing in IonBuilder.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

OK, here's take 2.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 730265 [details] [diff] [review]
v2

Review of attachment 730265 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonBuilder.cpp
@@ +6920,5 @@
>  
> +    MInstruction *ins;
> +    if (fun->isArrow()) {
> +        if (!info().fun())
> +            return abort("bound arrow function at toplevel");

nit: Add a comment above the if describing why we have this abort.

::: js/src/ion/MIR.h
@@ +3678,5 @@
> +    }
> +    JSFunction *fun() const {
> +        return fun_;
> +    }
> +    MDefinition *getThisValue() const {

nit: remove the get in this function name.

Comment 5

[Till Schneidereit [:till]]

Any particular reason why this hasn't landed?

Comment 6

[Jason Orendorff [:jorendorff]]

Ah er. Hmm. Well, it's super bitrotted. This was 9 months ago, it'll need a new review for sure.

(Note that this only makes ion able to emit a call to create an arrow function, without much of an eye for the speed of that operation, just trying not to kick the whole method out of ion. It doesn't help with calling arrow functions.)

It's a good cause though. Working on it.

Comment 7

[Guilherme Lima]

Is this a dupe of bug 988993?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Guilherme Lima from comment #7)
> Is this a dupe of bug 988993?

Yes, thanks.