1241088 - [SharedStubs] port NewArray and NewObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: 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.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Actual changes

Comment 3

[Eric Faust [:efaust]]

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

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

APPROVED.

Comment 4

[Eric Faust [:efaust]]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37d592080a03
https://hg.mozilla.org/integration/mozilla-inbound/rev/be2f6cb7251c

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37d592080a03
https://hg.mozilla.org/mozilla-central/rev/be2f6cb7251c