IonMonkey: support arrow functions

RESOLVED DUPLICATE of bug 988993

Status

()

defect
RESOLVED DUPLICATE of bug 988993
6 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

6 years ago
Posted patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #726350 - Flags: review?(nicolas.b.pierron)
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.
Attachment #726350 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 3

6 years ago
Posted patch v2Splinter Review
OK, here's take 2.
Attachment #726350 - Attachment is obsolete: true
Attachment #730265 - Flags: review?(nicolas.b.pierron)
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.
Attachment #730265 - Flags: review?(nicolas.b.pierron) → review+
Any particular reason why this hasn't landed?
Flags: needinfo?(jorendorff)
Assignee

Comment 6

6 years ago
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.
Flags: needinfo?(jorendorff)

Comment 7

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

Yes, thanks.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 988993
You need to log in before you can comment on or make changes to this bug.