[SharedStubs] port NewArray and NewObject

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Port the NewArray and NewObject stubs to sharedStubs and make them work for IonMonkey. This is based on top of the patches in bug 1234663. It also deduplicates/cleans some code in IonBuilder/MCallOptimize.
(Assignee)

Comment 1

3 years ago
Created attachment 8709938 [details] [diff] [review]
Part 1: Move the stubs from BaselineIC to SharedIC

This is just moving the files. It is just easier to rebase later to have these in another patch. It makes it also easier to review the next patch, which will only contain the actual changes.

@Eric: Jan has been reviewing all shared stub changes, but I've been submitting too many reviews to Jan, therefore looking for another victim ;). Should be relative straightforward, but feel free to nominate somebody else.
Assignee: nobody → hv1989
Attachment #8709938 - Flags: review?(efaustbmo)
(Assignee)

Comment 2

3 years ago
Created attachment 8709939 [details] [diff] [review]
Part 2: Actual changes
Attachment #8709939 - Flags: review?(efaustbmo)

Comment 3

3 years ago
Comment on attachment 8709938 [details] [diff] [review]
Part 1: Move the stubs from BaselineIC to SharedIC

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

APPROVED.
Attachment #8709938 - Flags: review?(efaustbmo) → review+

Comment 4

3 years ago
Comment on attachment 8709939 [details] [diff] [review]
Part 2: Actual changes

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

Minor nits. Otherwise, looks good to me.

::: js/src/jit/CodeGenerator.cpp
@@ +4635,4 @@
>          visitNewArrayCallVM(lir);
>          return;
>      }
> +    printf("non-template\n");

oops. Debug code left in here. Please remove.

::: js/src/jit/IonBuilder.cpp
@@ +7146,5 @@
> +{
> +    MOZ_ASSERT(*emitted == false);
> +
> +    // Emit a VM call.
> +    // Note: Can get removed if shared stubs is enabled by default.

"removed"?

@@ +7161,5 @@
> +    return true;
> +}
> +
> +bool
> +IonBuilder::jsop_newarray(uint32_t length)

This is much nicer...

@@ +7193,5 @@
> +
> +    if (!newArrayTryVM(&emitted, length) || emitted)
> +        return emitted;
> +
> +    MOZ_CRASH("newarray should have been emited");

Is there a reason we don't inline newArrayTryVM here at the end? Isn't that the convention in other jsop_* functions?

@@ +7309,5 @@
> +    }
> +    if (!newObjectTrySharedStub(&emitted) || emitted)
> +        return emitted;
> +
> +    if (!newObjectTryVM(&emitted) || emitted)

and here.

@@ +7406,5 @@
>              current->add(MPostWriteBarrier::New(alloc(), obj, value));
>  
> +        if ((obj->isNewArray() && obj->toNewArray()->convertDoubleElements()) ||
> +            (obj->isNullarySharedStub() &&
> +            obj->resultTypeSet()->convertDoubleElements(constraints()) == TemporaryTypeSet::AlwaysConvertToDoubles ))

nit: extra space inside parens at end of line
Attachment #8709939 - Flags: review?(efaustbmo) → review+
(Assignee)

Updated

2 years ago
Blocks: 1161516

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37d592080a03
https://hg.mozilla.org/mozilla-central/rev/be2f6cb7251c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1266579
(Assignee)

Updated

2 years ago
Depends on: 1272269
Depends on: 1328132
You need to log in before you can comment on or make changes to this bug.