GetArrayLength should be "blessed" during Fixup phase in the DFG

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

GetArrayLength should be "blessed" during Fixup phase in the DFG

[Keith Miller]

Created attachment 398688 [details]
Patch

[Keith Miller]

rdar://problem/62839448

[Mark Lam]

Comment on attachment 398688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398688&action=review

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        If we got an ArrayMode that expects to be configured during Fixup

Please add a comma after "Fixup".

[Keith Miller]

Created attachment 398822 [details]
Patch

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        GetArrayLength should be "blessed" during Fixup phase in the DFG

in your changelog, can you say what this fixes? You say some mechanical things, but not the original bug.

> Source/JavaScriptCore/ChangeLog:9
> +        If we got an ArrayMode that expects to be configured during Fixup,
> +        then right now we will fail. This fixes GetArrayLength to properly

Can you be a bit more specific on this first sentence. What you wrote is clearly not true.

> Source/JavaScriptCore/ChangeLog:22
> +        previously pass it's own speculation for the speculation of the index, which logically

it's => its

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:430
> +    if (isSomeTypedArrayView())
> +        return value.isType(SpecTypedArrayView);

how is this enough? e.g, how does this handle Float32Array vs Float64Array?

> Source/JavaScriptCore/dfg/DFGArrayMode.h:240
> +    static constexpr SpeculatedType unusedIndexSpeculatedType = SpecInt32Only;

alternatively:  just use Optional for the argument param

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

>> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:430
>> +        return value.isType(SpecTypedArrayView);
> 
> how is this enough? e.g, how does this handle Float32Array vs Float64Array?

I think this is correct if we're AnyTypedArray

But for specific typed arrays, we need a lot more than this.

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:247
> +        // As long as we have a JSArray getting its length shouldn't require any sane chainness.
> +        if (canBecomeGetArrayLength(graph, node) && isJSArray())
> +            return *this;

"Undecided" feels like the wrong name for this.

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:901
> +            speculationCheck(BadType, JSValueSource::unboxedCell(baseReg), 0, m_jit.branchIfNotType(baseReg, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView)));

nullptr, not zero. But why not provide the node?

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2031
> +                arrayMode = arrayMode.withType(Array::ForceExit);

can you also add an assert we can exit here?

[Saam Barati]

Comment on attachment 398822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398822&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2033
> +            blessArrayOperation(node->child1(), Edge(), node->child2());

Maybe we need to assert we're always ok to exit

[Keith Miller]

Created attachment 398872 [details]
Patch

[Saam Barati]

Comment on attachment 398872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398872&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        to be configured during Fixup, then right now we will fail for

still not obvious here what you mean by "fail"

> Source/JavaScriptCore/ChangeLog:26
> +        previously pass it's own speculation for the speculation of the index, which logically

it's => its

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:433
> +    if (isSomeTypedArrayView()) {
> +        if (type() == Array::AnyTypedArray)
> +            return value.isType(SpecTypedArrayView);
> +        return value.isType(speculationFromTypedArrayType(typedArrayType()));
> +    }

can you construct a test where your old code would've been broken?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2031
> +                arrayMode = arrayMode.withType(Array::ForceExit);

assert we can exit here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2032
> +            node->setArrayMode(arrayMode);

assert "isSpecific() here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:901
> +            speculationCheck(BadType, JSValueSource::unboxedCell(baseReg), 0, m_jit.branchIfNotType(baseReg, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView)));

nullptr, not zero

> Source/JavaScriptCore/jit/AssemblyHelpers.h:991
> +        if (last && *last != first) {
> +            ASSERT(*last > first);
> +            GPRReg scratch = scratchRegister();
> +            load8(Address(cellGPR, JSCell::typeInfoTypeOffset()), scratch);
> +            sub32(TrustedImm32(first), scratch);
> +            return branch32(Below, scratch, TrustedImm32(*last - first));
> +        }

this is unused. I'd revert your change here

> Source/JavaScriptCore/jit/AssemblyHelpers.h:996
> +    Jump branchIfNotType(GPRReg cellGPR, JSType first, Optional<JSType> last = WTF::nullopt)

You should comment that first/last is inclusive

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1003
> +            return branch32(AboveOrEqual, scratch, TrustedImm32(*last - first));

this is wrong, since "last" here is inclusive, you're going to say you're not the 'last' value AFAICT

[Saam Barati]

Comment on attachment 398872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398872&action=review

> Source/JavaScriptCore/runtime/JSType.h:134
>  static constexpr uint32_t LastTypedArrayType = DataViewType;

not your patch, but I feel like maybe we could name this better, and your new variable can subsume this name. "last" here reads to me like it should be inclusive, especially since "first" is

[Keith Miller]

Comment on attachment 398872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398872&action=review

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2031
>> +                arrayMode = arrayMode.withType(Array::ForceExit);
> 
> assert we can exit here.

That must be true because GetArrayLength clobbers exit state before Fixup as it's a ArrayMode node.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2032
>> +            node->setArrayMode(arrayMode);
> 
> assert "isSpecific() here?

That's not true because it can be ForceExit. :P But I can add ASSERT(isSpecific() || ForceExit).

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:901
>> +            speculationCheck(BadType, JSValueSource::unboxedCell(baseReg), 0, m_jit.branchIfNotType(baseReg, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView)));
> 
> nullptr, not zero

Done.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:991
>> +        }
> 
> this is unused. I'd revert your change here

I added it for symmetry.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:996
>> +    Jump branchIfNotType(GPRReg cellGPR, JSType first, Optional<JSType> last = WTF::nullopt)
> 
> You should comment that first/last is inclusive

Done.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:1003
>> +            return branch32(AboveOrEqual, scratch, TrustedImm32(*last - first));
> 
> this is wrong, since "last" here is inclusive, you're going to say you're not the 'last' value AFAICT

Yeah, you're right. I'll add at testmasm test for this.

>> Source/JavaScriptCore/runtime/JSType.h:134
>>  static constexpr uint32_t LastTypedArrayType = DataViewType;
> 
> not your patch, but I feel like maybe we could name this better, and your new variable can subsume this name. "last" here reads to me like it should be inclusive, especially since "first" is

Last is inclusive here. DataViews are JSArrayBufferViews so we include them in the list. We should probably make this more consistent throughout though.

[Keith Miller]

Created attachment 399041 [details]
Patch

[Keith Miller]

Created attachment 399043 [details]
Patch

[Saam Barati]

Comment on attachment 399043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399043&action=review

r=me. Seems like you need to rebase

> Source/JavaScriptCore/ChangeLog:9
> +        to be configured during Fixup, then right now we will fail for

Fail how?

> Source/JavaScriptCore/assembler/testmasm.cpp:2198
> +static void testBranchIfNotType()

You could also have a microbenchmark too

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2032
> +            node->setArrayMode(arrayMode);

It’s nice to have that assert you proposed. Maybe even in the DFG spec JIT / FTL lower. But here works too

[Keith Miller]

Created attachment 399152 [details]
Patch for landing

[Keith Miller]

Comment on attachment 399043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399043&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +        to be configured during Fixup, then right now we will fail for
> 
> Fail how?

Crash. Edited.

>> Source/JavaScriptCore/assembler/testmasm.cpp:2198
>> +static void testBranchIfNotType()
> 
> You could also have a microbenchmark too

This seems more consistent because if you "break" a microbenchmark we may or may not notice.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2032
>> +            node->setArrayMode(arrayMode);
> 
> It’s nice to have that assert you proposed. Maybe even in the DFG spec JIT / FTL lower. But here works too

Ok, I'll add it.

[Saam Barati]

Comment on attachment 399043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399043&action=review

>>> Source/JavaScriptCore/assembler/testmasm.cpp:2198
>>> +static void testBranchIfNotType()
>> 
>> You could also have a microbenchmark too
> 
> This seems more consistent because if you "break" a microbenchmark we may or may not notice.

"also", not "instead of"

[EWS]

Found 17 new test failures: imported/w3c/web-platform-tests/IndexedDB/idlharness.any.html, imported/w3c/web-platform-tests/html/dom/idlharness.https.html, imported/w3c/web-platform-tests/notifications/interfaces.html, imported/w3c/web-platform-tests/resize-observer/idlharness.window.html, imported/w3c/web-platform-tests/media-source/interfaces.html, imported/w3c/web-platform-tests/requestidlecallback/idlharness.window.html, imported/w3c/web-platform-tests/dom/idlharness.any.worker.html, imported/w3c/web-platform-tests/dom/idlharness.window.html, imported/w3c/web-platform-tests/css/cssom-view/idlharness.html, imported/w3c/web-platform-tests/xhr/idlharness.any.html, imported/w3c/web-platform-tests/xhr/idlharness.any.worker.html, imported/w3c/web-platform-tests/css/geometry/interfaces.worker.html, imported/w3c/web-platform-tests/fetch/api/idl.any.html, imported/w3c/web-platform-tests/resource-timing/idlharness.any.worker.html, js/dom/typed-array-set-different-types.html, fast/canvas/webgl/typed-arrays-in-workers.html, imported/w3c/web-platform-tests/fetch/api/idl.any.worker.html

[Keith Miller]

Created attachment 399184 [details]
Patch for landing

[Keith Miller]

(In reply to EWS from comment #21)
> Found 17 new test failures:
> imported/w3c/web-platform-tests/IndexedDB/idlharness.any.html,
> imported/w3c/web-platform-tests/html/dom/idlharness.https.html,
> imported/w3c/web-platform-tests/notifications/interfaces.html,
> imported/w3c/web-platform-tests/resize-observer/idlharness.window.html,
> imported/w3c/web-platform-tests/media-source/interfaces.html,
> imported/w3c/web-platform-tests/requestidlecallback/idlharness.window.html,
> imported/w3c/web-platform-tests/dom/idlharness.any.worker.html,
> imported/w3c/web-platform-tests/dom/idlharness.window.html,
> imported/w3c/web-platform-tests/css/cssom-view/idlharness.html,
> imported/w3c/web-platform-tests/xhr/idlharness.any.html,
> imported/w3c/web-platform-tests/xhr/idlharness.any.worker.html,
> imported/w3c/web-platform-tests/css/geometry/interfaces.worker.html,
> imported/w3c/web-platform-tests/fetch/api/idl.any.html,
> imported/w3c/web-platform-tests/resource-timing/idlharness.any.worker.html,
> js/dom/typed-array-set-different-types.html,
> fast/canvas/webgl/typed-arrays-in-workers.html,
> imported/w3c/web-platform-tests/fetch/api/idl.any.worker.html

Whoops, forgot to add the callback that says we don't want storage for TypedArray lengths.

[EWS]

Found 10 new test failures: imported/w3c/web-platform-tests/media-source/interfaces.html, imported/w3c/web-platform-tests/requestidlecallback/idlharness.window.html, imported/w3c/web-platform-tests/dom/idlharness.any.worker.html, imported/w3c/web-platform-tests/remote-playback/idlharness.window.html, imported/w3c/web-platform-tests/css/cssom-view/idlharness.html, imported/w3c/web-platform-tests/xhr/idlharness.any.html, imported/w3c/web-platform-tests/xhr/idlharness.any.worker.html, imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.html, imported/w3c/web-platform-tests/fetch/api/idl.any.html, imported/w3c/web-platform-tests/css/geometry/interfaces.html

[Keith Miller]

Created attachment 399395 [details]
Patch for landing

[EWS]

Committed r261712: <https://trac.webkit.org/changeset/261712>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399395 [details].