1322091 - Port remaining Baseline GETELEM stubs to CacheIR

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

After bug 1320670, the only non-CacheIR Baseline GETELEM stubs are for value[int32] with |value| one of {string, dense element, typed array, unboxed array, arguments}. It shouldn't be too hard to convert these.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Strings

This converts the string[int32] stub.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Magic arguments

I also folded the addPtr into the loadValue(BaseValueIndex.. following it.

I'll file a follow-up bug to avoid attaching duplicate stubs.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I'll file a follow-up bug to avoid attaching duplicate stubs.

Bug 1322320.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - ArgumentsObject

7 files changed, 79 insertions(+), 179 deletions(-)

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Dense element

8 files changed, 54 insertions(+), 125 deletions(-)

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Unboxed arrays

8 files changed, 64 insertions(+), 132 deletions(-)

Comment 7

[Tom S]

Comment on attachment 8817092 [details] [diff] [review]
Part 1 - Strings

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

::: js/src/jit/CacheIR.cpp
@@ +792,5 @@
> +    JSString* str = val_.toString();
> +    int32_t index = idVal_.toInt32();
> +    if (size_t(index) >= str->length() ||
> +        !str->isLinear() ||
> +        str->asLinear().latin1OrTwoByteChar(index) >= StaticStrings::UNIT_STATIC_LIMIT)

Good idea.

Comment 8

[Tom S]

Comment on attachment 8817106 [details] [diff] [review]
Part 2 - Magic arguments

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

::: js/src/jit/BaselineCacheIR.cpp
@@ +1616,5 @@
> +        return false;
> +
> +    // Bounds check.
> +    Address numActualArgs(BaselineFrameReg, BaselineFrame::offsetOfNumActualArgs());
> +    masm.loadPtr(Address(BaselineFrameReg, BaselineFrame::offsetOfNumActualArgs()), scratch);

Use numActualArgs

::: js/src/jit/CacheIR.cpp
@@ +839,5 @@
> +    MOZ_ASSERT(idVal_.isInt32());
> +
> +    if (!val_.isMagic(JS_OPTIMIZED_ARGUMENTS))
> +        return false;
> +

Can or should we check for the no FrameHasNoArgumentsObject before attaching?

Comment 9

[Tom S]

Comment on attachment 8817217 [details] [diff] [review]
Part 3 - ArgumentsObject

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

So much nicer!

::: js/src/jit/BaselineIC.cpp
@@ -1387,5 @@
> -    Label failureReconstructInputs;
> -    regs = availableGeneralRegs(0);
> -    regs.takeUnchecked(objReg);
> -    regs.takeUnchecked(idxReg);
> -    regs.take(scratchReg);

I am glad we don't need this anymore.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Typed arrays/objects

The last one.

For typed arrays/objects Baseline also supports object[double]. I added a check for int-valued doubles to GuardIsInt32, to convert the double to int32. The old code also converted -0 => 0 but I didn't do that as GuardIsInt32 is also used for other objects.

The nice thing is that this now works in all cases (like string[double] or array[double]).

Comment 11

[Tom S]

Comment on attachment 8817233 [details] [diff] [review]
Part 4 - Dense element

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

::: js/src/jit/CacheIR.cpp
@@ +886,5 @@
> +                                          ValOperandId indexId)
> +{
> +    MOZ_ASSERT(idVal_.isInt32());
> +
> +    if (!obj->isNative())

IsNativeDenseElementAccess checked for !is<TypedArrayObject>.

Comment 12

[Tom S]

Comment on attachment 8817281 [details] [diff] [review]
Part 6 - Typed arrays/objects

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

::: js/src/jit/BaselineCacheIR.cpp
@@ +955,5 @@
>          return false;
> +
> +    Label done;
> +    masm.branchTestInt32(Assembler::Equal, input, &done);
> +    {

I think we need a jitSupportsFloatingPoint check here.

::: js/src/jit/BaselineIC.cpp
@@ -936,5 @@
>  }
>  
>  static bool
> -TryAttachGetElemStub(JSContext* cx, JSScript* script, jsbytecode* pc, ICGetElem_Fallback* stub,
> -                     HandleValue lhs, HandleValue rhs, HandleValue res, bool* attached)

\o/

::: js/src/jit/CacheIR.cpp
@@ +956,5 @@
> +        return false;
> +
> +    TypedThingLayout layout = GetTypedThingLayout(obj->getClass());
> +    if (layout != Layout_TypedArray)
> +        writer.guardNoDetachedTypedObjects();

Where is this defined?

Comment 13

[Jan de Mooij [:jandem]]

Thanks for the quick reviews!

(In reply to Tom Schuster [:evilpie] from comment #8)
> Can or should we check for the no FrameHasNoArgumentsObject before attaching?

It's a bit annoying because we don't have the BaselineFrame* there. We could pass it but I don't think it's worth it. It seems fine to attach a stub for this edge case even if it will fail.

(In reply to Tom Schuster [:evilpie] from comment #11)
> IsNativeDenseElementAccess checked for !is<TypedArrayObject>.

Oh right, this is no longer necessary because we check index < initializedLength. Typed arrays don't have dense elements so the initialized length is always 0 and we will never attach a dense stub.

(In reply to Tom Schuster [:evilpie] from comment #12)
> I think we need a jitSupportsFloatingPoint check here.

Good catch!

> > +        writer.guardNoDetachedTypedObjects();
> 
> Where is this defined?

It's in the code already, I had to add it for something else (for typedObject.foo I think).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16720e74a36f
part 1 - Port Baseline string GETELEM stub to CacheIR. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3249dd222ca
part 2 - Port Baseline lazy arguments GETELEM stub to CacheIR. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff00ebb73274
part 3 - Port Baseline ArgumentsObject GETELEM stub to CacheIR. r=evilpie

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/16720e74a36f
https://hg.mozilla.org/mozilla-central/rev/b3249dd222ca
https://hg.mozilla.org/mozilla-central/rev/ff00ebb73274

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90dc3d44b941
part 4 - Port Baseline dense element GETELEM stub to CacheIR. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/516f8f487e6b
part 5 - Port Baseline unboxed array GETELEM stub to CacheIR. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe5d0073816
part 6 - Port Baseline TypedArray/TypedObject GETELEM stub to CacheIR. r=evilpie

Comment 17

[Tom S]

We should have a follow up about allowing to attach other stubs that use GuardIsInt32, if the index fits into int32.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90dc3d44b941
https://hg.mozilla.org/mozilla-central/rev/516f8f487e6b
https://hg.mozilla.org/mozilla-central/rev/1fe5d0073816

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #17)
> We should have a follow up about allowing to attach other stubs that use
> GuardIsInt32, if the index fits into int32.

Ah yes, definitely. Filed bug 1323096.