Last Comment Bug 1322091 - Port remaining Baseline GETELEM stubs to CacheIR
: Port remaining Baseline GETELEM stubs to CacheIR
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: unspecified
: All All
P1 normal with 1 vote (vote)
: mozilla53
Assigned To: Jan de Mooij [:jandem]
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: CacheIR
  Show dependency treegraph
 
Reported: 2016-12-04 22:49 PST by Jan de Mooij [:jandem]
Modified: 2016-12-12 16:24 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Strings (17.24 KB, patch)
2016-12-06 18:29 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review
Part 2 - Magic arguments (10.65 KB, patch)
2016-12-06 19:29 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review
Part 3 - ArgumentsObject (18.81 KB, patch)
2016-12-07 12:43 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review
Part 4 - Dense element (15.46 KB, patch)
2016-12-07 13:33 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review
Part 5 - Unboxed arrays (16.46 KB, patch)
2016-12-07 14:02 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review
Part 6 - Typed arrays/objects (25.34 KB, patch)
2016-12-07 15:36 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2016-12-04 22:49:55 PST
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 User image Jan de Mooij [:jandem] 2016-12-06 18:29:44 PST
Created attachment 8817092 [details] [diff] [review]
Part 1 - Strings

This converts the string[int32] stub.
Comment 2 User image Jan de Mooij [:jandem] 2016-12-06 19:29:14 PST
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.
Comment 3 User image Jan de Mooij [:jandem] 2016-12-06 19:37:57 PST
(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 User image Jan de Mooij [:jandem] 2016-12-07 12:43:30 PST
Created attachment 8817217 [details] [diff] [review]
Part 3 - ArgumentsObject

 7 files changed, 79 insertions(+), 179 deletions(-)
Comment 5 User image Jan de Mooij [:jandem] 2016-12-07 13:33:45 PST
Created attachment 8817233 [details] [diff] [review]
Part 4 - Dense element

 8 files changed, 54 insertions(+), 125 deletions(-)
Comment 6 User image Jan de Mooij [:jandem] 2016-12-07 14:02:30 PST
Created attachment 8817240 [details] [diff] [review]
Part 5 - Unboxed arrays

 8 files changed, 64 insertions(+), 132 deletions(-)
Comment 7 User image Tom Schuster [:evilpie] 2016-12-07 15:07:37 PST
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 User image Tom Schuster [:evilpie] 2016-12-07 15:22:52 PST
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 User image Tom Schuster [:evilpie] 2016-12-07 15:35:45 PST
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 User image Jan de Mooij [:jandem] 2016-12-07 15:36:23 PST
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]).
Comment 11 User image Tom Schuster [:evilpie] 2016-12-07 18:12:47 PST
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 User image Tom Schuster [:evilpie] 2016-12-08 11:11:28 PST
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 User image Jan de Mooij [:jandem] 2016-12-08 17:10:32 PST
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 User image Pulsebot 2016-12-09 14:27:01 PST
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
Comment 16 User image Pulsebot 2016-12-11 23:27:32 PST
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
Comment 17 User image Tom Schuster [:evilpie] 2016-12-12 02:34:26 PST
We should have a follow up about allowing to attach other stubs that use GuardIsInt32, if the index fits into int32.
Comment 19 User image Jan de Mooij [:jandem] 2016-12-12 16:24:03 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.