IonMonkey: Generate typed array stubs for SetElementIC

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

26.64 KB, patch
jandem
: review+
Details | Diff | Splinter Review
15.95 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.17 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
20.47 KB, patch
jandem
: review+
Details | Diff | Splinter Review
76.18 KB, patch
decoder
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Would be nice.
(Assignee)

Updated

5 years ago
QA Contact: shu
(Assignee)

Updated

5 years ago
Assignee: general → shu
QA Contact: shu
(Assignee)

Comment 1

5 years ago
Created attachment 785021 [details] [diff] [review]
Part 1: Refactor value-to-int logic into masm

Refactors the logic for converting constant and runtime values to int into the macro assembler so they can be used from both within ICs and the codegen.
Attachment #785021 - Flags: review?(jdemooij)
(Assignee)

Comment 2

5 years ago
Created attachment 785023 [details] [diff] [review]
Part 2: SetElementIC typed array stubs

Stubs for setting typed array elements.
Attachment #785023 - Flags: review?(jdemooij)
Comment on attachment 785021 [details] [diff] [review]
Part 1: Refactor value-to-int logic into masm

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

Nice to have all this logic in one place..

::: js/src/ion/IonMacroAssembler.cpp
@@ +1489,5 @@
> +
> +    Label done, isInt32, isBool, isDouble, isNull, isString;
> +
> +    if (maybeInput) {
> +        branchEqualTypeIfNeeded(MIRType_Int32, maybeInput, tag, &isInt32);

Can we make branchEqualType always emit the branch if the MDefinition is NULL? that would avoid some duplication here.

@@ +1519,5 @@
> +    if (isString.used()) {
> +        bind(&isString);
> +        Register str = extractString(value, stringReg);
> +        if (str != stringReg)
> +            movePtr(str, stringReg);

unboxString(value, stringReg);

I see unboxString in MacroAssembler-x64.h and defining it for x86/ARM should be easy (just like unboxInt32).

@@ +1629,5 @@
> +        break;
> +      case MIRType_Boolean:
> +      case MIRType_Int32:
> +        if (src.typedReg().gpr() != output)
> +            movePtr(src.typedReg().gpr(), output);

move32
Attachment #785021 - Flags: review?(jdemooij) → review+
Comment on attachment 785023 [details] [diff] [review]
Part 2: SetElementIC typed array stubs

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

Looks good but some comments below.. Also, we should temporarily disable the inline typed array code in IonBuilder and run jit-tests (ideally there would be a shell flag to do that, also for the other ICs, but can be done in another bug)

::: js/src/ion/IonCaches.cpp
@@ +2770,5 @@
> +    Label failures;
> +
> +    // See note in GenerateGetTypedArrayElement; clasp discriminates typed arrays.
> +    int arrayType = tarr->type();
> +    masm.branchTestObjClass(Assembler::NotEqual, object, temp0, tarr->getClass(), &failures);

For dense elements, we just check the shape instead of the class and that's also what the Baseline set-typed-array-element stub does. This works because the shape determines the class. It's probably faster to do that here too (same class different shapes should be very uncommon for typed arrays and checking the Class requires some extra loads).

@@ +2778,5 @@
> +    Register index = masm.extractInt32(indexVal, temp0);
> +
> +    // Guard on the length.
> +    Address length(object, TypedArrayObject::lengthOffset());
> +    masm.branch32(Assembler::BelowOrEqual, length, index, &failures);

The typed array length is stored as boxed Int32Value. This may work but I think it's cleaner to use masm.unboxInt32(length, temp0) as we do elsewhere.

Also, OOB typed array writes are always no-ops so we can just make this jump to a done label.

@@ +2796,5 @@
> +            return false;
> +        masm.storeToTypedFloatArray(arrayType, tempUnbox, target);
> +    } else {
> +        // Use the object register as the output register.
> +        Register output = object;

We can't clobber the object register; the next LIR instruction will expect the original object there. Can we use temp0 here?

::: js/src/ion/IonCaches.h
@@ +713,5 @@
>      ValueOperand index_;
>      ConstantOrRegister value_;
>      bool strict_;
>  
>      bool hasDenseStub_ : 1;

Pre-existing and should be fixed in another bug, but this doesn't make sense anymore -- we want to be able to attach different dense stubs for different shapes.

It's from the dense array days, all dense arrays had the same shape. We don't really have dense arrays anymore, just dense elements, and it's possible to have 2 objects with dense elements but different shapes.

@@ +714,5 @@
>      ConstantOrRegister value_;
>      bool strict_;
>  
>      bool hasDenseStub_ : 1;
> +    int16_t hasTypedArrayStub_;

Considering the previous comment, is this really necessary? It would be nice if we could remove it and guard on the shape instead.

@@ +770,5 @@
> +        return hasTypedArrayStub_ & (1 << arrayType);
> +    }
> +    void setHasTypedArrayStub(int arrayType) {
> +        JS_ASSERT(!hasTypedArrayStub(arrayType));
> +        hasTypedArrayStub_ |= (1 << arrayType);

JS_ASSERT(hasTypedArrayStub(arrayType));

After setting hasTypedArrayStub_ so that it will complain if 1 << arrayType does not fit in int16_t.
Attachment #785023 - Flags: review?(jdemooij)
(Assignee)

Comment 5

5 years ago
Created attachment 795745 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v2

Applied review comments
Attachment #785023 - Attachment is obsolete: true
Attachment #795745 - Flags: review?(jdemooij)
Comment on attachment 795745 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v2

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

::: js/src/jit/Lowering.cpp
@@ +2527,5 @@
>  
>          if (!useBox(lir, LSetElementCacheV::Index, ins->index()))
>              return false;
>          if (!useBox(lir, LSetElementCacheV::Value, ins->value()))
>              return false;

It's unfortunate, but this will cause problems on x86. Normally we have 7 registers available, but if the profiler is enabled we don't allocate ebp, so that leaves 6 registers. This instruction requires 7 registers (1 for the object, 2 for the index, 2 for the value, 2 temps)... See also bug 900890, it added the tempToUnbox() call to fix this.

(We should probably also add an assert to regalloc to catch this even if the profiler is disabled. Or run jit-tests with the profiler enabled.)
Attachment #795745 - Flags: review?(jdemooij)
(Assignee)

Comment 7

5 years ago
Created attachment 796367 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v3

To work around only having 6 registers, I'm pushing/popping the object register on x86, and using an "unbox" temp otherwise.

I thought originally I could just do extractInt32 on the length, but we're loading the length from memory, not a ValueOperand, so there's no register to reuse.

It's kind of dirty with the ifdefs, but I don't see another solution.
Attachment #795745 - Attachment is obsolete: true
Attachment #796367 - Flags: review?(jdemooij)
Comment on attachment 796367 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v3

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

This looks really great, just one comment/question/thought.

::: js/src/jit/IonCaches.cpp
@@ +2928,5 @@
> +        if (!masm.convertConstantOrRegisterToDouble(cx, value, tempFloat, &failures))
> +            return false;
> +        masm.storeToTypedFloatArray(arrayType, tempFloat, target);
> +    } else {
> +#ifdef JS_CPU_X86

tempToUnbox is only valid on x64 so we should do this also on ARM.

How about always pushing the object, even on x64? It's a bit less efficient but (1) tempToUnbox() is really intended for unboxing only and using it here feels a bit wrong, (2) it would also avoid a temp register that's very typed-array specific and (3) it would avoid the #ifdefs. What do you think?
(Assignee)

Comment 9

5 years ago
(In reply to Jan de Mooij [:jandem] from comment #8)

> How about always pushing the object, even on x64? It's a bit less efficient
> but (1) tempToUnbox() is really intended for unboxing only and using it here
> feels a bit wrong, (2) it would also avoid a temp register that's very
> typed-array specific and (3) it would avoid the #ifdefs. What do you think?

Okay, I think I'm convinced by (2), we can always push the object.
(Assignee)

Comment 10

5 years ago
Created attachment 796882 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v4

Always push object.
Attachment #796367 - Attachment is obsolete: true
Attachment #796367 - Flags: review?(jdemooij)
Attachment #796882 - Flags: review?(jdemooij)
Comment on attachment 796882 [details] [diff] [review]
Part 2: SetElementIC typed array stubs v4

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

Nice!
Attachment #796882 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 797497 [details] [diff] [review]
Part 3: Install the IC for typed array writes

Currently setelem ICs are only installed if baseline ICs witnessed a dense write. Change it so we also install ICs for typed array writes.

Also populated JSID_VOID heap typesets for typed arrays to convince Ion the writes to typed arrays do not need a type barrier. Not sure if this is the preferred approach; bhackett, please advise.
Attachment #797497 - Flags: review?(bhackett1024)
Comment on attachment 797497 [details] [diff] [review]
Part 3: Install the IC for typed array writes

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

Can you attach a new patch with the comment addressed?

::: js/src/jsinfer.cpp
@@ +3522,5 @@
> +          case ScalarTypeRepresentation::TYPE_FLOAT64:
> +            base->types.addType(cx, Type::DoubleType());
> +            break;
> +        }
> +    }

TI doesn't provide any information about property types for non-native objects, including typed arrays.  Rather than extending that invariant, Ion should just be smarter when deciding whether a property write needs a type barrier and special case writes to typed arrays, similar to what we do for reads from typed arrays.
Attachment #797497 - Flags: review?(bhackett1024)
(Assignee)

Comment 14

5 years ago
Created attachment 798098 [details] [diff] [review]
Part 3: Install the IC for typed array writes v2

v2 without the incorrect change to TypeObject::getProperty
Attachment #797497 - Attachment is obsolete: true
Attachment #798098 - Flags: review?(bhackett1024)
Comment on attachment 798098 [details] [diff] [review]
Part 3: Install the IC for typed array writes v2

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

::: js/src/jit/IonBuilder.cpp
@@ +7228,5 @@
>          return true;
>  
>      // TODO: Bug 876650: remove this check:
>      // Temporary disable the cache if non dense native,
>      // untill the cache supports more ics

preexisting, but typo here

::: js/src/jit/MIR.cpp
@@ +2815,5 @@
> +        ScalarTypeRepresentation::Type arrayType =
> +            (ScalarTypeRepresentation::Type) object->getTypedArrayType();
> +        if (arrayType < ScalarTypeRepresentation::TYPE_MAX && JSID_IS_VOID(id)) {
> +            if (MIRTypeFromScalarType(arrayType) != (*pvalue)->type())
> +                return true;

From TI's perspective there are no meaningful property types in a typed array, and never a need for barriers --- writing to the array converts the written value into whatever the array's element type is.  Can you remove this test, then?  I don't know if removing this will allow the IC to generate writes of random things to the typed array, so SetElementIC::canAttachTypedArrayElement might need to be modified with similar logic to this, but that is the appropriate place for it.
Attachment #798098 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #15)
> Comment on attachment 798098 [details] [diff] [review]
> Part 3: Install the IC for typed array writes v2
> 
> Review of attachment 798098 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +7228,5 @@
> >          return true;
> >  
> >      // TODO: Bug 876650: remove this check:
> >      // Temporary disable the cache if non dense native,
> >      // untill the cache supports more ics
> 
> preexisting, but typo here

I'll fix it.

> 
> ::: js/src/jit/MIR.cpp
> @@ +2815,5 @@
> > +        ScalarTypeRepresentation::Type arrayType =
> > +            (ScalarTypeRepresentation::Type) object->getTypedArrayType();
> > +        if (arrayType < ScalarTypeRepresentation::TYPE_MAX && JSID_IS_VOID(id)) {
> > +            if (MIRTypeFromScalarType(arrayType) != (*pvalue)->type())
> > +                return true;
> 
> From TI's perspective there are no meaningful property types in a typed
> array, and never a need for barriers --- writing to the array converts the
> written value into whatever the array's element type is.  Can you remove
> this test, then?  I don't know if removing this will allow the IC to
> generate writes of random things to the typed array, so
> SetElementIC::canAttachTypedArrayElement might need to be modified with
> similar logic to this, but that is the appropriate place for it.

Good point, I'll remove the type check and the corresponding helper function (MIRTypeFromScalarType), but there still needs to be a check in |PropertyWriteNeedsTypeBarrier| so it doesn't return true for typed arrays:

        // TI doesn't track TypedArray objects and should never insert a type
        // barrier for them.
        if (object->getTypedArrayType() < ScalarTypeRepresentation::TYPE_MAX)
            continue;

Any objections to that? Lifting that logic out of |PropertyWriteNeedsTypeBarrier| requires duplicating the logic of the first part of the function -- I don't see a reason to do that.
(Assignee)

Comment 17

5 years ago
Created attachment 798389 [details] [diff] [review]
fuzz.patch

Gary, could you fuzz this combined patch for me?
Attachment #798389 - Flags: feedback?(gary)
Attachment #798389 - Flags: feedback?(choller)
Depends on: 911821
Depends on: 911897
Comment on attachment 798389 [details] [diff] [review]
fuzz.patch

The following tests fail (m-c rev d54e0cce6c17, options --ion-eager):

function testClampInt() {
    var arr = new Uint8ClampedArray(100);
        for (var j=0; j<values.length; j++) {
            arr[1] = values[j];
    }
}
testClampInt();


Assertion failure: !fails.used(), at js/src/jit/CodeGenerator.cpp:6614

===

for (var i=0; '' & inSection(30); i++) {}


Crashes on Heap (this blocks any further testing).

===

function test() {
  var arr = new Uint8ClampedArray();
  for (var i = 0; i < .0 ; i++)
    arr[i] = "Infinity";
} test();


Crashes [@ saveLive].
Attachment #798389 - Flags: feedback?(choller) → feedback-
Comment on attachment 798389 [details] [diff] [review]
fuzz.patch

function g() {
    try {
        f()
    } catch (e) {}
}
function m(code) {
    try {
        f = Function(code)
    } catch (r) {}
    g()
}
m("x=new Uint8ClampedArray")
m("x[0]=h")

(Here's another testcase that causes Assertion failure: !fails.used(), at jit/CodeGenerator.cpp )
Attachment #798389 - Flags: feedback?(gary) → feedback-
(Assignee)

Comment 20

5 years ago
Created attachment 799141 [details] [diff] [review]
fuzz.patch

Fixed those crashes, please re-fuzz.
Attachment #798389 - Attachment is obsolete: true
Attachment #799141 - Flags: feedback?(choller)
Attachment #799141 - Flags: feedback?(gary)
(Assignee)

Comment 21

5 years ago
Created attachment 799242 [details] [diff] [review]
fuzz.patch

Small fix for something that got lost during refactoring.
Attachment #799141 - Attachment is obsolete: true
Attachment #799141 - Flags: feedback?(gary)
Attachment #799141 - Flags: feedback?(choller)
Attachment #799242 - Flags: feedback?(gary)
Attachment #799242 - Flags: feedback?(choller)
Comment on attachment 799242 [details] [diff] [review]
fuzz.patch

Nothing found overnight, tested with patch in comment 20.
Attachment #799242 - Flags: feedback?(gary) → feedback+
Comment on attachment 799242 [details] [diff] [review]
fuzz.patch

The following test fails (m-c rev d54e0cce6c17, options --ion-eager):


function testBasicTypedArrays() {
    var ar, aridx, idx;
    var a = new Uint8Array(16);
    var b = new Uint16Array(16);
    var c = new Uint32Array(16);
    var d = new Int8Array(16);
    var e = new Int16Array(16);
    var f = new Int32Array(16);
    var g = new Float32Array(16);
    var h = new Float64Array(16);
    var iarrays = [ a, b, c, d, e, f ];
    for (aridx = 0; aridx < iarrays.length; ++aridx) {
        ar = iarrays[aridx];
        for (idx = 0; idx < ar.length-4; ++idx) {
            ar[idx] = 22;
        }
    }
    var farrays = [ g, h ];
}
testBasicTypedArrays();


Assertion failure: !byteRegRequiresRex(reg), at ../assembler/assembler/X86Assembler.h:3267



I'm seeing another GC-related failure but I'm still reducing that one.
Attachment #799242 - Flags: feedback?(choller) → feedback-
(Assignee)

Comment 24

5 years ago
Created attachment 800372 [details] [diff] [review]
Part 4: Add useByteOpRegister for lowering

Fixes the x86 formatter crash and refactors the architecture-specific visitStoreTypedArrayElement{Hole} in Lowering.

Note that for length, the ARM lowering code for StoreTypedArray used |useRegisterOrConstant| for length while all other architecture lowerings used |useAnyOrConstant|. Turns out that |useAnyOrConstant| is already defined to just call |useRegisterOrConstant| on ARM.
Attachment #800372 - Flags: review?(jdemooij)
(Assignee)

Comment 25

5 years ago
Created attachment 800407 [details] [diff] [review]
fuzz.patch

Fixed the x86 formatter crash.
Attachment #799242 - Attachment is obsolete: true
Attachment #800407 - Flags: feedback?(gary)
Attachment #800407 - Flags: feedback?(choller)
Comment on attachment 800372 [details] [diff] [review]
Part 4: Add useByteOpRegister for lowering

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

Nice fix!
Attachment #800372 - Flags: review?(jdemooij) → review+
Comment on attachment 800407 [details] [diff] [review]
fuzz.patch

Nothing found overnight either.
Attachment #800407 - Flags: feedback?(gary) → feedback+
Comment on attachment 800407 [details] [diff] [review]
fuzz.patch

No further issues found, sorry for the delay :)
Attachment #800407 - Flags: feedback?(choller) → feedback+
Depends on: 914478
You need to log in before you can comment on or make changes to this bug.