983675 - Generally inline all TypeDescr predicates

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

TypeDescrIsSimpleType
TypeDescrIsSizedArrayType

There are others.  Look for them where the definitions are for the two above.

Comment 1

[Lars T Hansen [:lth]]

Closely related to Bug 977177.

Comment 2

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #1)
> Closely related to Bug 977177.

Make that Bug 977117.

Comment 3

[Lars T Hansen [:lth]]

These are on the sequential setup path for TypedObject's mapPar:

- TypeOfTypedObject               [a JS function]
- TypeDescrIsArrayType
- TypeDescrIsUnsizedArrayType     [presumably this will disappear]
- ObjectIsTypeDescr               [already inlined]
- TypeDescrIsSimpleType
- ObjectIsTransparentTypedObject  [already inlined]
- ObjectIsTypedObject             [a JS function calling inlined functions]

Comment 4

[Lars T Hansen [:lth]]

Attachment: typeDescrArrayPredicates.patch

In principle something like this ought to be sufficient -- especially if we nail down the type desriptor magic values and then improve the testing beyond a two-arm or three arm logical 'or' -- but i'm having a hard time verifying that the tests are being inlined and optimized properly.

Comment 5

[Niko Matsakis [:nmatsakis]]

I think what's missing from that patch is that ion can't constant propagate the kind as we wrote it today. I see two basic options here:

(1) Introduce an intrinsic for loading the KIND of a typed object, or else have ion recognize and inline calls to UnsafeGetReservedSlot() that are being used on a typed object with the JS_DESCR_KIND_SLOT slot. We can then keep the tests in JS code and let constant propagation and inlining handle it for us.

(2) Introduce custom intrinsics for these tests rather than coding them in self-hosted code.

Either seems ok.

Comment 6

[Niko Matsakis [:nmatsakis]]

I was thinking on this some more. I have some slight preference for defining intrinsics, I think, because:

(1) It seems less obvious to me that we would specialize UnsafeGetReservedSlot() to yield a constant.
(2) An intrinsic can sometimes be constant propagated where the other technique cannot.

I am imagining a test like TypeDescrIsSimple(), which can be propagated to true as long as *all* the known classes are simple type descriptor classes and propagated to false as long as *none* of the known classes are simple type descriptor classes. In contrast, we can only specialize UnsafeGetReservedSlot() for KIND loads if we know the precise type descriptor class.

Of course, I do expect we will know the precise class in most cases of interest, but it still seems that the intrinsic approach is more flexible, and also a bit less surprising to me.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Niko Matsakis [:nmatsakis] from comment #6)
> (1) It seems less obvious to me that we would specialize
> UnsafeGetReservedSlot() to yield a constant.

Agree.

> (2) An intrinsic can sometimes be constant propagated where the other
> technique cannot.

I'll look into this; certainly it makes sense.

(My problem yesterday was to make the code that performs the test matter enough to the JIT for it to optimize it in the first place, or so it seemed).

Comment 8

[Lars T Hansen [:lth]]

Attachment: Patch

This is basically straightforward, with two wrinkles.

One is that it's hard to test these intrinsics because they're not exposed; we could use some sort of white-box test facility (either expose intrinsics in the shell or create a mode in the shell that runs built-in tests).  So the tests are sort of fiddly and require examining the iongraph output.

The other is the expansion I've chosen for testing an object against multiple class pointers: it tests against all the pointers and ORs the results together.  To my eye the resulting machine code is pretty decent and not worse than the code we'd get from introducing control flow, and it's branch-free.  And once the JIT learns how to common the loading of the class pointer there will be additional savings.  But I'd appreciate to hear about other perspectives on this; I could be trying to be too clever.

Comment 9

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8393568 [details] [diff] [review]
Patch

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

I didn't read deeply, but this looks nice to me. I agree about the difficulty in writing tests, it's a pain!

One thought is that we could expose the intrinsics in the TestingFunctions setup, perhaps.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8393568 [details] [diff] [review]
Patch

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

The branch-free ORing is certainly fine by me! My main concern here are the tests. jit-tests is at the end of the day an automated test suite. Adding tests there that require manual inspection is just not going to be useful. It's too bad, but I would remove all the tests that don't assert/check anything. Keep them around elsewhere, perhaps, but they won't be very useful inside jit-tests.

r=me with style nits and the testing situation addressed.

::: js/src/builtin/TypedObject.cpp
@@ +2732,5 @@
> +    JS_ASSERT(args.length() == 1);
> +    JS_ASSERT(args[0].isObject());
> +    JS_ASSERT(args[0].toObject().is<js::TypeDescr>());
> +    const Class *clasp = args[0].toObject().getClass();
> +    args.rval().setBoolean(clasp == &SizedArrayTypeDescr::class_ || clasp == &UnsizedArrayTypeDescr::class_);

Use is<T> instead of pulling out the clasp manually.

Also style nit: SpiderMonkey uses 100-char lines.

@@ +2747,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() == 1);
> +    JS_ASSERT(args[0].isObject());
> +    JS_ASSERT(args[0].toObject().is<js::TypeDescr>());
> +    args.rval().setBoolean(args[0].toObject().getClass() == &SizedArrayTypeDescr::class_);

Ditto.

@@ +2762,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() == 1);
> +    JS_ASSERT(args[0].isObject());
> +    JS_ASSERT(args[0].toObject().is<js::TypeDescr>());
> +    args.rval().setBoolean(args[0].toObject().getClass() == &UnsizedArrayTypeDescr::class_);

Ditto.

::: js/src/builtin/TypedObject.h
@@ +724,5 @@
>  extern const JSJitInfo ObjectIsTransparentTypedObjectJitInfo;
>  
> +/* Predicates on type descriptor objects.  In all cases, 'obj' must be a type descriptor. */
> +
> +bool TypeDescrIsSimpleType(ThreadSafeContext *, unsigned argc, Value *vp);

Nit: name the cx, here and below.

::: js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsArrayType.js
@@ +26,5 @@
> +    return w;
> +}
> +
> +var w = test();
> +print(w.length);

This doesn't seem like a useful automated test. I would remove it.

::: js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsSimpleType.js
@@ +24,5 @@
> +    return w;
> +}
> +
> +var w = test();
> +print(w.length);

Remove.

::: js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsSizedArrayType.js
@@ +29,5 @@
> +    return w;
> +}
> +
> +var w = test();
> +print(w.length);

Remove.

::: js/src/jit/MCallOptimize.cpp
@@ +158,5 @@
>          return inlineToObject(callInfo);
>  
>      // TypedObject intrinsics.
> +    if (native == intrinsic_ObjectIsTypedObject)
> +        return inlineHasClasses(callInfo, &TransparentTypedObject::class_, &OpaqueTypedObject::class_);

100-char line.

@@ +168,5 @@
>          return inlineObjectIsTypeDescr(callInfo);
> +    if (native == intrinsic_TypeDescrIsSimpleType)
> +        return inlineHasClasses(callInfo, &ScalarTypeDescr::class_, &ReferenceTypeDescr::class_);
> +    if (native == intrinsic_TypeDescrIsArrayType)
> +        return inlineHasClasses(callInfo, &SizedArrayTypeDescr::class_, &UnsizedArrayTypeDescr::class_);

100-char line.

@@ +1580,5 @@
> +        MHasClass *hasClass1 = MHasClass::New(alloc(), callInfo.getArg(0), clasp1);
> +        current->add(hasClass1);
> +        if (clasp2 == nullptr) {
> +            current->push(hasClass1);
> +        }

Style nit: SpiderMonkey doesn't use braces around single-line bodies. It'll probably bite us one day, but style rules are style rules...

@@ +1582,5 @@
> +        if (clasp2 == nullptr) {
> +            current->push(hasClass1);
> +        }
> +        else {
> +            // The following turns into branch-free, box-free code on x86, and should do so on ARM.

Nice!

@@ +1588,5 @@
> +            current->add(hasClass2);
> +            MBitOr *either = MBitOr::New(alloc(), hasClass1, hasClass2);
> +            either->infer(inspector, pc);
> +            current->add(either);
> +            MNot *n1 = MNot::New(alloc(), either);

Perhaps rename n1/n2 to something a bit more descriptive?

Comment 11

[Lars T Hansen [:lth]]

Attachment: Patch v2

Nits fixed.  Tests made useful (by adding assertEq broadly).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36f90c0041a6

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/36f90c0041a6