Patch coming.
Created attachment 264559 [details] the patch.
Created attachment 264560 [details] 32-bit benchmark result 1
Created attachment 264561 [details] 32-bit benchmark result 2
Created attachment 264562 [details] 64-bit benchmark result 1
Created attachment 264563 [details] 64-bit benchmark result 2
Created attachment 264564 [details] Added missing snippet files.
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 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 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.
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 on attachment 264605 [details] Patch 2: Fixed bug and incorporated feedback. EWS is red.
Created attachment 264610 [details] patch 3: svn up'ed
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 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 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
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
Thanks for the review. Feedback is applied. Landed in r191905: <http://trac.webkit.org/r191905>.
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
(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.