[JSC] Implement $vm.ftlTrue function for FTL testing

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Mark Lam]

Let's name it $vm.ftlTrue instead.

[Yusuke Suzuki]

(In reply to Mark Lam from comment #1)
> Let's name it $vm.ftlTrue instead.

Sounds good.

[Yusuke Suzuki]

Created attachment 334536 [details]
Patch

[Mark Lam]

Comment on attachment 334536 [details]
Patch

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

r=me with suggestions.

> Source/JavaScriptCore/jsc.cpp:-292
>  static EncodedJSValue JSC_HOST_CALL functionFalse1(ExecState*);
> -static EncodedJSValue JSC_HOST_CALL functionFalse2(ExecState*);

You can rename functionFalse1 to functionFalse now that there's only one.

> Source/JavaScriptCore/jsc.cpp:518
> -        putDirectNativeFunction(vm, this, Identifier::fromString(&vm, "isFinalTier"), 0, functionFalse2, IsFinalTierIntrinsic, static_cast<unsigned>(PropertyAttribute::DontEnum));
> +        putDirectNativeFunction(vm, this, Identifier::fromString(&vm, "isFinalTier"), 0, functionFalse1, IsFinalTierIntrinsic, static_cast<unsigned>(PropertyAttribute::DontEnum));

Ditto.  Use functionFalse.

> Source/JavaScriptCore/jsc.cpp:-1831
>  EncodedJSValue JSC_HOST_CALL functionFalse1(ExecState*) { return JSValue::encode(jsBoolean(false)); }
> -EncodedJSValue JSC_HOST_CALL functionFalse2(ExecState*) { return JSValue::encode(jsBoolean(false)); }

Ditto.  Use functionFalse.

> JSTests/stress/has-indexed-property-array-storage-ftl.js:8
> +var flag = false;

I suggest renaming "flag" to "didFTLCompile".

> JSTests/stress/has-indexed-property-slow-put-array-storage-ftl.js:8
> +var flag = false;

I suggest renaming "flag" to "didFTLCompile".

[Yusuke Suzuki]

Comment on attachment 334536 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/jsc.cpp:-292
>> -static EncodedJSValue JSC_HOST_CALL functionFalse2(ExecState*);
> 
> You can rename functionFalse1 to functionFalse now that there's only one.

Nice, fixed.

>> Source/JavaScriptCore/jsc.cpp:518
>> +        putDirectNativeFunction(vm, this, Identifier::fromString(&vm, "isFinalTier"), 0, functionFalse1, IsFinalTierIntrinsic, static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> Ditto.  Use functionFalse.

Fixed.

>> Source/JavaScriptCore/jsc.cpp:-1831
>> -EncodedJSValue JSC_HOST_CALL functionFalse2(ExecState*) { return JSValue::encode(jsBoolean(false)); }
> 
> Ditto.  Use functionFalse.

Fixed.

>> JSTests/stress/has-indexed-property-array-storage-ftl.js:8
>> +var flag = false;
> 
> I suggest renaming "flag" to "didFTLCompile".

Nice. Changed.

>> JSTests/stress/has-indexed-property-slow-put-array-storage-ftl.js:8
>> +var flag = false;
> 
> I suggest renaming "flag" to "didFTLCompile".

Changed.

[Yusuke Suzuki]

Committed r228950: <https://trac.webkit.org/changeset/228950>

[Radar WebKit Bug Importer]

<rdar://problem/37829272>

[Alexey Proskuryakov]

32-bit test failures on new tests:

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1263/steps/webkit-32bit-jsc-test/logs/stdio

stress/has-indexed-property-array-storage-ftl.js.misc-ftl-no-cjit: Exception: Error: bad value: false
stress/has-indexed-property-array-storage-ftl.js.misc-ftl-no-cjit: [e-mail]:5:24
stress/has-indexed-property-array-storage-ftl.js.misc-ftl-no-cjit: global [e-mail]:22:9
stress/has-indexed-property-array-storage-ftl.js.misc-ftl-no-cjit: ERROR: Unexpected exit code: 3

[Mark Lam]

(In reply to Alexey Proskuryakov from comment #8)
> 32-bit test failures on new tests:

the FTL is not supported on 32-bit.  Hence, these are guaranteed to fail.  We should skip these tests on 32-bit.

[Alexey Proskuryakov]

Please do skip them then!

[Yusuke Suzuki]

Committed r228986: <https://trac.webkit.org/changeset/228986>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #11)
> Committed r228986: <https://trac.webkit.org/changeset/228986>

OK, skipped!

[Yusuke Suzuki]

Committed r229057: <https://trac.webkit.org/changeset/229057>