Port remaining Baseline GETELEM stubs to CacheIR

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

5 months ago
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.
(Assignee)

Comment 1

5 months ago
Created attachment 8817092 [details] [diff] [review]
Part 1 - Strings

This converts the string[int32] stub.
Attachment #8817092 - Flags: review?(evilpies)
(Assignee)

Comment 2

5 months ago
Created attachment 8817106 [details] [diff] [review]
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.
Attachment #8817106 - Flags: review?(evilpies)
(Assignee)

Comment 3

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

Bug 1322320.
(Assignee)

Comment 4

5 months ago
Created attachment 8817217 [details] [diff] [review]
Part 3 - ArgumentsObject

 7 files changed, 79 insertions(+), 179 deletions(-)
Attachment #8817217 - Flags: review?(evilpies)
(Assignee)

Comment 5

5 months ago
Created attachment 8817233 [details] [diff] [review]
Part 4 - Dense element

 8 files changed, 54 insertions(+), 125 deletions(-)
Attachment #8817233 - Flags: review?(evilpies)
Priority: -- → P1
(Assignee)

Comment 6

5 months ago
Created attachment 8817240 [details] [diff] [review]
Part 5 - Unboxed arrays

 8 files changed, 64 insertions(+), 132 deletions(-)
Attachment #8817240 - Flags: review?(evilpies)
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.
Attachment #8817092 - Flags: review?(evilpies) → review+
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?
Attachment #8817106 - Flags: review?(evilpies) → review+
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.
Attachment #8817217 - Flags: review?(evilpies) → review+
(Assignee)

Comment 10

5 months ago
Created attachment 8817281 [details] [diff] [review]
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]).
Attachment #8817281 - Flags: review?(evilpies)
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>.
Attachment #8817233 - Flags: review?(evilpies) → review+
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?
Attachment #8817281 - Flags: review?(evilpies) → review+

Updated

5 months ago
Attachment #8817240 - Flags: review?(evilpies) → review+
(Assignee)

Comment 13

5 months ago
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

5 months ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

5 months ago
Keywords: leave-open

Comment 15

4 months ago
bugherder
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

4 months ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

4 months ago
Keywords: leave-open
We should have a follow up about allowing to attach other stubs that use GuardIsInt32, if the index fits into int32.

Comment 18

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90dc3d44b941
https://hg.mozilla.org/mozilla-central/rev/516f8f487e6b
https://hg.mozilla.org/mozilla-central/rev/1fe5d0073816
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 19

4 months ago
(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.
You need to log in before you can comment on or make changes to this bug.