Bug 171269 – [JSC] Math unary functions should be handled by DFG

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 308090 [details]
Patch

[Yusuke Suzuki]

Created attachment 308105 [details]
Patch

[Build Bot]

Attachment 308105 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:43:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:56:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:111:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:978:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2193:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 5 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 308366 [details]
Patch

[Yusuke Suzuki]

Created attachment 308367 [details]
Patch

[Build Bot]

Attachment 308367 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:43:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:56:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:111:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2193:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 4 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 308367 [details]
Patch

Attachment 308367 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3618439

New failing tests:
stress/regress-159779-1.js.ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/math.js.layout-dfg-eager-no-cjit
stress/regress-159779-2.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-llint
stress/regress-159779-2.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/math.js.layout
stress/regress-159779-1.js.default
jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-ftl
stress/regress-159779-1.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/math.js.layout-ftl-eager-no-cjit
stress/regress-159779-2.js.ftl-no-cjit
stress/regress-159779-1.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/math.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-cjit
stress/regress-159779-2.js.default
ChakraCore.yaml/ChakraCore/test/es6/ES6MathAPIs.js.default

[Build Bot]

Comment on attachment 308367 [details]
Patch

Attachment 308367 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3618547

New failing tests:
js/math.html

[Build Bot]

Created attachment 308374 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308367 [details]
Patch

Attachment 308367 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3618551

New failing tests:
js/math.html

[Build Bot]

Created attachment 308375 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308367 [details]
Patch

Attachment 308367 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3618556

New failing tests:
js/math.html

[Build Bot]

Created attachment 308376 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 308378 [details]
Patch

[Build Bot]

Attachment 308378 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:43:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:56:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:111:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2193:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 4 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 308410 [details]
Patch

[Build Bot]

Attachment 308410 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:43:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:56:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArithMode.cpp:111:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2193:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 4 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 308410 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        optimization phase. Actually, ArithLog is effective in kraken.

what do you mean by effective here? We effectively optimize it in Kraken?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2186
> +        if (argumentCountIncludingThis == 1) {
> +            insertChecks();
> +            set(VirtualRegister(resultOperand), addToGraph(JSConstant, OpInfo(m_constantNaN)));
> +            return true;
> +        }

Do we have tests for all of these?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:406
> +    double a = op1.toNumber(exec); \

nit: Lets pick a better name than "a".

> Source/JavaScriptCore/runtime/MathCommon.cpp:528
> +    if (!value)

Nit: the older version had ==0 here, I think that's more readable.

[Yusuke Suzuki]

Comment on attachment 308410 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:12
>> +        optimization phase. Actually, ArithLog is effective in kraken.
> 
> what do you mean by effective here? We effectively optimize it in Kraken?

ArithLog was introduced to optimize Kraken. That makes ArithLog pure and encourages code motion.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2186
>> +        }
> 
> Do we have tests for all of these?

Nice, added.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:406
>> +    double a = op1.toNumber(exec); \
> 
> nit: Lets pick a better name than "a".

Changed it to result.

>> Source/JavaScriptCore/runtime/MathCommon.cpp:528
>> +    if (!value)
> 
> Nit: the older version had ==0 here, I think that's more readable.

OK, changed.

[Yusuke Suzuki]

Committed r216178: <http://trac.webkit.org/changeset/216178>