1111164 - The UnsafeGetReservedSlot intrinsic should allow the caller to communicate information about the type of the value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, the codegen for something like this (from ArrayIteratorNext):

658     var a = UnsafeGetReservedSlot(this, ARRAY_ITERATOR_SLOT_ITERATED_OBJECT);
659     var index = UnsafeGetReservedSlot(this, ARRAY_ITERATOR_SLOT_NEXT_INDEX);
660     var itemKind = UnsafeGetReservedSlot(this, ARRAY_ITERATOR_SLOT_ITEM_KIND); 

ends up looking like so:

[LoadFixedSlotV]
    movq       0x20(%rax), %rcx
[Unbox:Object]
    movq       %rcx, %r11
    shrq       $47, %r11
    cmpl       $0x1fff8, %r11d
    jne        ((2478))
    movabsq    $0x7fffffffffff, %r11
    movq       %rcx, %rcx
    andq       %r11, %rcx
[MoveGroup]
    movq       %rcx, 0x48(%rsp)
[TypeBarrierO]
    movq       0x8(%rcx), %rdx
    movabsq    $0x132aa6eb0, %r11
    cmpq       %r11, %rdx
    jne        ((2522))
[LoadFixedSlotV]
    movq       0x28(%rax), %rdx
[Unbox:Int32]
    movq       %rdx, %r11
    shrq       $47, %r11
    cmpl       $0x1fff1, %r11d
    jne        ((2546))
    movl       %edx, %edx
[MoveGroup]
    movl       %edx, 0x54(%rsp)
[LoadFixedSlotV]
    movq       0x30(%rax), %rbx
[Unbox:Int32]
    movq       %rbx, %r11
    shrq       $47, %r11
    cmpl       $0x1fff1, %r11d
    jne        ((2576))
    movl       %ebx, %ebx

All those tag checks are a bit annoying.  In particular, we know that ARRAY_ITERATOR_SLOT_ITERATED_OBJECT will in fact return an object and that ARRAY_ITERATOR_SLOT_ITEM_KIND will return a boxed integer (in fact, one of 0, 1, 2).   Things are a bit tougher for ARRAY_ITERATOR_SLOT_NEXT_INDEX because it could be an int or a double...

In any case, for the two cases when we do know the types, we should be able to unbox infallibly.  A possible implementation approach:

1)  Expose MIRType or JSValueType equivalent to self-hosted JS in some way.
2)  Change UnsafeGetReservedSlot to take an optional third argument indicating the type in
    the slot.
3)  Have the actual intrinsic MOZ_ASSERT that the passed-in type matches the type of the
    Value in the slot.
4)  In IonBuilder::inlineUnsafeGetReservedSlot, if a type is passed in, go ahead and insert
    an infallible MUnbox to that type before doing pushTypeBarrier.  Then the type barrier
    code will skip the unbox, and we'll get infallible unboxes here.

Any reason we shouldn't do this?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Hackeypatch to just unbox infallibly to the observed return type, for demonstration purposes

Comment 2

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #0)
> Any reason we shouldn't do this?

None, obviously.  Just be careful that we're always acting on fully-initialized objects with all reserved slots properly filled in.  Reading out just values is slower but mildly safer.

> 1)  Expose MIRType or JSValueType equivalent to self-hosted JS in some way.
> 2)  Change UnsafeGetReservedSlot to take an optional third argument
> indicating the type in
>     the slot.

One maybe-elegant way to do this, that occurred to me, might be to have the caller pass in a constant of the desired type.  Codegen could then use that as the type to expect/assert as being identical (and assert the argument's always a constant, too).

> Things are a bit tougher for ARRAY_ITERATOR_SLOT_NEXT_INDEX
> because it could be an int or a double...

The above trick wouldn't handle this case, and I'm not sure what would.  The alternative of having multiple UnsafeGetReservedSlotT intrinsics is always an option -- just one I assume we want to avoid to not have a bunch of duplicated code.  Maybe that's solvable with a single implementation under the hood, in which case separate intrinsics seems the best, least tricksy way.  (The above way is certainly tricksy.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> might be to have the caller pass in a constant of the desired type

Getting your hands quickly (as in, 0-cost after jit compilation) on an object-typed constant is not trivial, right?

> The above trick wouldn't handle this case, and I'm not sure what would.

The only thing I can think of that would is if we always stored the slot as a double or some such.  But in practice, we don't actually want to do that...

Separate intrinsics would actually not be that hard, now that you mention it.  We could in fact share implementations without too much difficulty.  The hardest part here is bikeshedding the naming.

The immediate use-cases I have would want UnsafeGetObjectFromReservedSlot and UnsafeGetInt32FromReservedSlot or some such.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We need to fix bug 924059 first, because otherwise the ArrayIterator prototype can leak into our code and make us not happy campers because its slots are not inited.

I'll post the patches I have anyway.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add infrastructure for doing typed reserved slot gets in self-hosted code and having them resulting unboxing be unconditional in Ion

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch to typed reserved slot gets in Array.js

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch to typed reserved slot gets in TypedObject.js

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch to typed reserved slot gets in String.js

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch to typed reserved slot gets in TypedObject.js

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8535977 [details] [diff] [review]
part 1.  Add infrastructure for doing typed reserved slot gets in self-hosted code and having them resulting unboxing be unconditional in Ion

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

Nice idea.

::: js/src/jit/MCallOptimize.cpp
@@ +2086,5 @@
> +        // what we've seen coming from this slot in the past, then do an
> +        // infallible unbox and barrier on the unbox result.  That way the type
> +        // barrier code won't end up doing MIRType checks and conditional
> +        // unboxing.
> +        MOZ_ASSERT(getInlineReturnType() == knownValueType);

If we have never executed this call in Baseline, this assert probably won't hold because the (observed) return type set will be empty and I think you'll get MIRType_Value here. This should work:

MOZ_ASSERT_IF(!getInlineReturnTypeSet()->empty(), ...);

@@ +2088,5 @@
> +        // barrier code won't end up doing MIRType checks and conditional
> +        // unboxing.
> +        MOZ_ASSERT(getInlineReturnType() == knownValueType);
> +        loadResult = MUnbox::New(alloc(), load, knownValueType, MUnbox::Infallible);
> +        current->add(loadResult);

We can do even better by loading an unboxed value directly instead of loading a Value and then unboxing:

  load->setResultType(knownValueType);

This should result in LLoadFixedSlotT instead of LLoadFixedSlotV.

@@ +2095,3 @@
>  
>      // We don't track reserved slot types, so always emit a barrier.
> +    if (!pushTypeBarrier(loadResult, getInlineReturnTypeSet(), BarrierKind::TypeSet))

Can you verify we don't actually emit a barrier for int32/bool/string loads? For objects it's hard to avoid.

::: js/src/vm/SelfHosting.cpp
@@ +591,5 @@
> +js::intrinsic_UnsafeGetObjectFromReservedSlot(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    if (!intrinsic_UnsafeGetReservedSlot(cx, argc, vp))
> +        return false;
> +    MOZ_ASSERT(vp->isObject());

Great asserts.

Comment 11

[Tom S [:evilpie]]

Comment on attachment 8535980 [details] [diff] [review]
part 4.  Switch to typed reserved slot gets in String.js

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

::: js/src/builtin/String.js
@@ +211,5 @@
>      if (!IsObject(this) || !IsStringIterator(this))
>          ThrowError(JSMSG_INCOMPATIBLE_METHOD, "StringIterator", "next", ToString(this));
>  
> +    var S = UnsafeGetStringFromReservedSlot(this, STRING_ITERATOR_SLOT_ITERATED_STRING);
> +    // The index might be int or double, so we have to do an untyped get.

Can it really? We have a primitive string here, so length can only be somewhere around 1<<27, which should even with +2 go outside the int32 range.

Comment 12

[Tom S [:evilpie]]

* not go

Comment 13

[Boris Zbarsky [:bzbarsky]]

> If we have never executed this call in Baseline, this assert probably won't hold

Good catch.  Fixed.

> We can do even better by loading an unboxed value directly 

Oh, nice.  My patch had things like:

[LoadFixedSlotV]
    movq       0x30(%rax), %rdi
[Unbox:Int32]
    movl       %edi, %edi

but with your suggestion I just get:

[LoadFixedSlotT]
    movl       0x30(%rax), %edi

which is definitely even nicer.

> Can you verify we don't actually emit a barrier for int32/bool/string loads?

Yep, I did.  At least according to JIT inspector.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> so length can only be somewhere around 1<<27

Hmm.  I guess we could depend on that here, if we add the right asserts somewhere.  Someone other than me should make that call.

Comment 15

[Jeff Walden [:Waldo]]

For String objects it seems totally fine to depend upon the length limit holding.  Please do add a static_assert(JSString::MAX_LENGTH <= INT32_MAX, "StringIteratorNext in builtin/String.js assumes the stored index in the string is an Int32Value"); to SelfHosting.cpp or so (and a symmetric comment in String.js pointing to the assert), to ensure this assumption/dependency is guarded against, tho.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8535980 [details] [diff] [review]
part 4.  Switch to typed reserved slot gets in String.js

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

+1 but I agree with Tom.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  Switched that one to an int32 load, added the assert and comment suggested in comment 15 and:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2660a3ca47
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1691e1db088
https://hg.mozilla.org/integration/mozilla-inbound/rev/96ec553f0f8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e27e73cca9fc

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6a2660a3ca47
https://hg.mozilla.org/mozilla-central/rev/f1691e1db088
https://hg.mozilla.org/mozilla-central/rev/96ec553f0f8f
https://hg.mozilla.org/mozilla-central/rev/e27e73cca9fc