Bug 150129

Summary: Snippefy op_add for the baseline JIT.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, commit-queue, fpizlo, ggaren, keith_miller, msaboff, rniwa, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
none
32-bit benchmark result 1
none
32-bit benchmark result 2
none
64-bit benchmark result 1
none
64-bit benchmark result 2
none
Added missing snippet files.
ggaren: review-
Patch 2: Fixed bug and incorporated feedback.
ggaren: review-
patch 3: svn up'ed
ggaren: review+, buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-mavericks none

Description Mark Lam 2015-10-14 10:55:13 PDT 
Patch coming.
Comment 1 Mark Lam 2015-11-01 23:58:45 PST 
Created attachment 264559 [details]
the patch.
Comment 2 Mark Lam 2015-11-02 00:00:52 PST 
Created attachment 264560 [details]
32-bit benchmark result 1
Comment 3 Mark Lam 2015-11-02 00:01:16 PST 
Created attachment 264561 [details]
32-bit benchmark result 2
Comment 4 Mark Lam 2015-11-02 00:01:39 PST 
Created attachment 264562 [details]
64-bit benchmark result 1
Comment 5 Mark Lam 2015-11-02 00:02:05 PST 
Created attachment 264563 [details]
64-bit benchmark result 2
Comment 6 Mark Lam 2015-11-02 00:09:57 PST 
Created attachment 264564 [details]
Added missing snippet files.
Comment 7 WebKit Commit Bot 2015-11-02 00:12:24 PST 
Attachment 264564 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:57:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:59:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:61:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:62:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:63:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:64:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:65:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:66:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 11 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Saam Barati 2015-11-02 07:26:28 PST 
Comment on attachment 264564 [details]
Added missing snippet files.

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

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:44
> +    if (!m_leftType.mightBeNumber() && !m_rightType.mightBeNumber())

Should this be && or ||?
It looks like the old code was ||

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:45
> +        m_slowPathJumpList.append(jit.jump());

I think you should return here because any code you emit after this is unreachable
Comment 9 Geoffrey Garen 2015-11-02 10:21:28 PST 
Comment on attachment 264564 [details]
Added missing snippet files.

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

>> Source/JavaScriptCore/jit/JITAddGenerator.cpp:44
>> +    if (!m_leftType.mightBeNumber() && !m_rightType.mightBeNumber())
> 
> Should this be && or ||?
> It looks like the old code was ||

Yeah, this needs to be ||.

> Source/JavaScriptCore/jit/JITAddGenerator.h:49
> +        // Hence, we can never have BothAreConstInt32.

It would be better just to say that we choose not to optimize the both constant case. Saying we can never have it suggests that we rely on it never happening. But we shouldn't rely on an optimization for correctness here.
Comment 10 Mark Lam 2015-11-02 11:41:44 PST 
Created attachment 264605 [details]
Patch 2: Fixed bug and incorporated feedback.

Thanks for the reviews, and for catching the bug.  Both Saam and Geoff's feedback have been incorporated.  I changed the comment in JITAddGenerator.h as Geoff suggested, and added a comment to the ChangeLog instead to document why I don't implement the optimization for the 2 const operands case.
Comment 11 Geoffrey Garen 2015-11-02 11:43:51 PST 
Comment on attachment 264605 [details]
Patch 2: Fixed bug and incorporated feedback.

EWS is red.
Comment 12 Mark Lam 2015-11-02 11:57:44 PST 
Created attachment 264610 [details]
patch 3: svn up'ed
Comment 13 WebKit Commit Bot 2015-11-02 11:59:39 PST 
Attachment 264610 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:57:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:59:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:60:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:61:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:62:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:63:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:64:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:65:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:66:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 11 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Geoffrey Garen 2015-11-02 12:17:18 PST 
Comment on attachment 264610 [details]
patch 3: svn up'ed

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

r=me

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:50
> +        // Try to do intConstant + intVar.

This is intVar + intConstant.

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:51
> +        CCallHelpers::Jump tryDoubleAdd = jit.branchIfNotInt32(m_left);

Let's call this notInt32 since it does not always try to do a double add.

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:63
> +        // Try to do double(intConstant) + doubleVar.

doubleVar + double(intConstant)
Comment 15 Build Bot 2015-11-02 12:41:40 PST 
Comment on attachment 264610 [details]
patch 3: svn up'ed

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

New failing tests:
storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures.html
Comment 16 Build Bot 2015-11-02 12:41:43 PST 
Created attachment 264615 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 17 Mark Lam 2015-11-02 13:13:54 PST 
Thanks for the review.  Feedback is applied.  Landed in r191905: <http://trac.webkit.org/r191905>.
Comment 18 Michael Saboff 2015-11-02 19:31:02 PST 
Looks like this broke some tests on iOS ARM.  These tests started failing with r191905:

jsc-layout-tests.yaml/js/script-tests/dfg-int16array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-int32array-overflow-values.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-int32array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-int8array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-uint16array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-uint32array-overflow-values.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-uint32array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-uint8array.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/dfg-uint8clampedarray.js.layout-no-llint
Comment 19 Mark Lam 2015-11-03 15:54:02 PST 
(In reply to comment #18)
> Looks like this broke some tests on iOS ARM.  These tests started failing
> with r191905:
> ...

All of these were due to the op_add snippet increasing generated code size.  Turns out that there op_add snippets have some inefficiencies that I've now fixed in http://trac.webkit.org/changeset/191978.  With that fix, these tests now pass.