Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(434)

Issue 1426913002: [Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes (Closed)

Created:
5 years, 1 month ago by mythria
Modified:
5 years, 1 month ago
Reviewers:
oth, rmcilroy
CC:
v8-reviews_googlegroups.com, oth
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Interpreter] Merges ToBoolean and JumpIfTrue/False bytecodes Adds an optimization to emit JumpIfToBooleanTrue/False instead of ToBoolean followed by JumpIfTrue/False if the value in the accumulator is not boolean. BUG=v8:4280 LOG=N Committed: https://crrev.com/e66d4f87360bed9f61e1d74298eb1b05f7bad0df Cr-Commit-Position: refs/heads/master@{#31697}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed review comments #

Patch Set 3 : Last patch was not complete. Forgot few changes. #

Total comments: 12

Patch Set 4 : Addressed review comments #

Patch Set 5 : rebased the patch and used the new Repeat_32 macro for tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+508 lines, -146 lines) Patch
M src/interpreter/bytecode-array-builder.h View 1 2 3 4 3 chunks +3 lines, -5 lines 0 comments Download
M src/interpreter/bytecode-array-builder.cc View 1 2 3 4 3 chunks +59 lines, -37 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 4 chunks +2 lines, -4 lines 0 comments Download
M test/cctest/interpreter/test-bytecode-generator.cc View 1 2 3 4 8 chunks +230 lines, -53 lines 0 comments Download
M test/cctest/interpreter/test-interpreter.cc View 1 2 3 4 4 chunks +67 lines, -6 lines 0 comments Download
M test/unittests/interpreter/bytecode-array-builder-unittest.cc View 1 2 3 4 4 chunks +147 lines, -41 lines 0 comments Download

Messages

Total messages: 19 (6 generated)
mythria
Could you please review the changes. Thanks and Regards, Mythri
5 years, 1 month ago (2015-10-28 14:23:09 UTC) #3
rmcilroy
Looks pretty good. Please also add: BUG=v8:4280 LOG=N to the description (and for all future ...
5 years, 1 month ago (2015-10-29 10:40:25 UTC) #4
mythria
Addressed comments from Ross. Could you review the changes. Thanks and Regards, Mythri https://codereview.chromium.org/1426913002/diff/1/src/interpreter/bytecode-array-builder.cc File ...
5 years, 1 month ago (2015-10-29 17:58:39 UTC) #7
rmcilroy
LGTM with comments (but please wait for Orion to review too). https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecode-array-builder.cc File src/interpreter/bytecode-array-builder.cc (right): ...
5 years, 1 month ago (2015-10-30 10:36:00 UTC) #8
oth
lgtm. A minor comment about naming, but super. https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecode-array-builder.h File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecode-array-builder.h#newcode237 src/interpreter/bytecode-array-builder.h:237: bool ...
5 years, 1 month ago (2015-10-30 10:46:32 UTC) #9
rmcilroy
https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc File test/cctest/interpreter/test-bytecode-generator.cc (right): https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc#newcode499 test/cctest/interpreter/test-bytecode-generator.cc:499: X X X X X X X X X ...
5 years, 1 month ago (2015-10-30 10:50:07 UTC) #10
mythria
Thanks for reviews. I addressed all comments. https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecode-array-builder.cc File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1426913002/diff/40001/src/interpreter/bytecode-array-builder.cc#newcode460 src/interpreter/bytecode-array-builder.cc:460: // the ...
5 years, 1 month ago (2015-10-30 11:44:47 UTC) #11
rmcilroy
https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc File test/cctest/interpreter/test-bytecode-generator.cc (right): https://codereview.chromium.org/1426913002/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc#newcode499 test/cctest/interpreter/test-bytecode-generator.cc:499: X X X X X X X X X ...
5 years, 1 month ago (2015-10-30 12:09:53 UTC) #12
mythria
Along with rebasing the patch, I did following changes: 1. fixed test-bytecode-generator to use the ...
5 years, 1 month ago (2015-10-30 14:19:08 UTC) #13
rmcilroy
> 1. fixed test-bytecode-generator to use the new Repeat_32 macro along with > rebase. Great ...
5 years, 1 month ago (2015-10-30 14:42:52 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426913002/80001
5 years, 1 month ago (2015-10-30 16:47:13 UTC) #17
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 1 month ago (2015-10-30 16:48:22 UTC) #18
commit-bot: I haz the power
5 years, 1 month ago (2015-10-30 16:48:43 UTC) #19
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/e66d4f87360bed9f61e1d74298eb1b05f7bad0df
Cr-Commit-Position: refs/heads/master@{#31697}

Powered by Google App Engine
This is Rietveld 408576698