Bug 171269 - [JSC] Math unary functions should be handled by DFG
Summary: [JSC] Math unary functions should be handled by DFG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-25 04:40 PDT by Yusuke Suzuki
Modified: 2017-05-04 04:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (43.43 KB, patch)
2017-04-25 04:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (151.40 KB, patch)
2017-04-25 09:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (162.98 KB, patch)
2017-04-27 02:56 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (160.46 KB, patch)
2017-04-27 02:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (980.56 KB, application/zip)
2017-04-27 04:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (924.34 KB, application/zip)
2017-04-27 04:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.79 MB, application/zip)
2017-04-27 04:36 PDT, Build Bot
no flags Details
Patch (160.50 KB, patch)
2017-04-27 04:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (160.50 KB, patch)
2017-04-27 11:11 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-04-25 04:40:22 PDT
...
Comment 1 Yusuke Suzuki 2017-04-25 04:52:42 PDT
Created attachment 308090 [details]
Patch
Comment 2 Yusuke Suzuki 2017-04-25 09:16:53 PDT
Created attachment 308105 [details]
Patch
Comment 3 Build Bot 2017-04-25 09:19:52 PDT
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.
Comment 4 Yusuke Suzuki 2017-04-27 02:56:39 PDT
Created attachment 308366 [details]
Patch
Comment 5 Yusuke Suzuki 2017-04-27 02:59:42 PDT
Created attachment 308367 [details]
Patch
Comment 6 Build Bot 2017-04-27 03:01:05 PDT
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.
Comment 7 Build Bot 2017-04-27 04:02:59 PDT
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
Comment 8 Build Bot 2017-04-27 04:25:05 PDT
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
Comment 9 Build Bot 2017-04-27 04:25:07 PDT
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
Comment 10 Build Bot 2017-04-27 04:28:45 PDT
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
Comment 11 Build Bot 2017-04-27 04:28:46 PDT
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
Comment 12 Build Bot 2017-04-27 04:36:31 PDT
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
Comment 13 Build Bot 2017-04-27 04:36:33 PDT
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
Comment 14 Yusuke Suzuki 2017-04-27 04:52:27 PDT
Created attachment 308378 [details]
Patch
Comment 15 Build Bot 2017-04-27 04:54:58 PDT
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.
Comment 16 Yusuke Suzuki 2017-04-27 11:11:12 PDT
Created attachment 308410 [details]
Patch
Comment 17 Build Bot 2017-04-27 11:13:14 PDT
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.
Comment 18 Yusuke Suzuki 2017-05-01 16:09:41 PDT
Ping?
Comment 19 Saam Barati 2017-05-03 12:20:50 PDT
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.
Comment 20 Yusuke Suzuki 2017-05-04 04:28:25 PDT
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.
Comment 21 Yusuke Suzuki 2017-05-04 04:40:56 PDT
Committed r216178: <http://trac.webkit.org/changeset/216178>