Closed Bug 1241088 Opened 5 years ago Closed 4 years ago

[SharedStubs] port NewArray and NewObject

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files)

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.
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)
Attachment #8709939 - Flags: review?(efaustbmo)
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 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+
Blocks: 1161516
https://hg.mozilla.org/mozilla-central/rev/37d592080a03
https://hg.mozilla.org/mozilla-central/rev/be2f6cb7251c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1272269
You need to log in before you can comment on or make changes to this bug.