Bug 150129 – Snippefy op_add for the baseline JIT.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 264559 [details]
the patch.

[Mark Lam]

Created attachment 264560 [details]
32-bit benchmark result 1

[Mark Lam]

Created attachment 264561 [details]
32-bit benchmark result 2

[Mark Lam]

Created attachment 264562 [details]
64-bit benchmark result 1

[Mark Lam]

Created attachment 264563 [details]
64-bit benchmark result 2

[Mark Lam]

Created attachment 264564 [details]
Added missing snippet files.

[WebKit Commit Bot]

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.

[Saam Barati]

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

[Geoffrey Garen]

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.

[Mark Lam]

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.

[Geoffrey Garen]

Comment on attachment 264605 [details]
Patch 2: Fixed bug and incorporated feedback.

EWS is red.

[Mark Lam]

Created attachment 264610 [details]
patch 3: svn up'ed

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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)

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Thanks for the review.  Feedback is applied.  Landed in r191905: <http://trac.webkit.org/r191905>.

[Michael Saboff]

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

[Mark Lam]

(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.