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

Issue 1291693004: [Interpreter] Bytecode graph builder (Closed)

Created:
5 years, 4 months ago by oth
Modified:
5 years, 3 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Interpreter] Skeleton bytecode graph builder Add skeleton version bytecode-graph-builder.{h,cc} for existing bytecodes. BUG=v8:4280 LOG=N Committed: https://crrev.com/8df7b4f6b502e2c318b61ce604332d51544081c6 Cr-Commit-Position: refs/heads/master@{#30687}

Patch Set 1 #

Patch Set 2 : Progress bytecode graph builder and add dummy cctest as driver for bytecode graph builder. #

Patch Set 3 : Remove dead unittest. #

Patch Set 4 : Rebase and enable in pipeline.cc #

Patch Set 5 : Remove dead code. #

Total comments: 18

Patch Set 6 : Incorporate review comments on patch set 5. #

Total comments: 4

Patch Set 7 : Eliminate CreateGraphForTest, add support for parameters, misc fixes, rebase. #

Patch Set 8 : Fixes/progress from rmcilroy to enable turbofan to process graphs from bytecode. #

Total comments: 33

Patch Set 9 : Incorporate comments on patch set 8. #

Total comments: 19

Patch Set 10 : Incorporate comments on patch set 9. #

Total comments: 4

Patch Set 11 : Attempt to straighten out parameter setup. #

Total comments: 1

Patch Set 12 : Add new bytecode-graph-builder unit test, drop temporary cctest and rebase. #

Patch Set 13 : Add cctest for functions with parameters. #

Patch Set 14 : Additional tests with strings and double. #

Total comments: 23

Patch Set 15 : Incorporate review comments on patch set 14. #

Patch Set 16 : Rebase and add VisitStoreIC and VisitKeyedStoreIC to fix compilation. #

Patch Set 17 : Attempt to fix gn build. #

Patch Set 18 : More fixes for bots (tests w/ ASAN and tests w/ slow asserts enabled). #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1371 lines, -5 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -0 lines 0 comments Download
M src/DEPS View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A src/compiler/bytecode-graph-builder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +174 lines, -0 lines 0 comments Download
A src/compiler/bytecode-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +433 lines, -0 lines 2 comments Download
M src/compiler/pipeline.cc View 1 2 3 2 chunks +15 lines, -4 lines 0 comments Download
A src/interpreter/bytecode-array-iterator.h View 1 2 3 4 5 6 7 8 9 1 chunk +45 lines, -0 lines 0 comments Download
A src/interpreter/bytecode-array-iterator.cc View 1 2 3 4 5 6 7 8 9 1 chunk +72 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M test/cctest/cctest.gyp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
A test/cctest/compiler/test-run-bytecode-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +259 lines, -0 lines 0 comments Download
M test/cctest/interpreter/test-bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -1 line 0 comments Download
A test/unittests/compiler/bytecode-graph-builder-unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +248 lines, -0 lines 0 comments Download
M test/unittests/compiler/node-test-utils.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M test/unittests/compiler/node-test-utils.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
A test/unittests/interpreter/bytecode-array-iterator-unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +100 lines, -0 lines 0 comments Download
M test/unittests/unittests.gyp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 38 (7 generated)
oth
mstarzinger@chromium.org rmcilroy@chromium.org titzer@chromium.org PTAL. This is just a start. Input on wiring up graphs for ...
5 years, 4 months ago (2015-08-20 16:18:06 UTC) #2
oth
On 2015/08/20 16:18:06, oth wrote: > mailto:mstarzinger@chromium.org > mailto:rmcilroy@chromium.org > mailto:titzer@chromium.org > > PTAL. This ...
5 years, 3 months ago (2015-09-01 12:25:48 UTC) #3
rmcilroy
Looking good to me. I don't have enough knowledge on TF to comment on the ...
5 years, 3 months ago (2015-09-01 14:00:06 UTC) #4
oth
Thanks! https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode21 src/compiler/bytecode-graph-builder.cc:21: // NB Nodes talk slides: http://shortn/_fmVf0TjCC4 On 2015/09/01 ...
5 years, 3 months ago (2015-09-01 15:25:58 UTC) #5
Michael Starzinger
Looks sensible from a high-level point of view. No concrete actionable feedback at this point. ...
5 years, 3 months ago (2015-09-02 12:19:22 UTC) #6
rmcilroy
https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22 src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph); On 2015/09/01 15:25:57, oth wrote: > On ...
5 years, 3 months ago (2015-09-02 12:45:24 UTC) #7
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22 src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph); On 2015/09/02 12:45:24, rmcilroy wrote: > On ...
5 years, 3 months ago (2015-09-02 12:46:53 UTC) #8
Benedikt Meurer
Graph creation skeleton looks good to me.
5 years, 3 months ago (2015-09-03 05:02:55 UTC) #10
oth
Thanks all. With Ross's help we can run simple graphs from the bytecode graph builder ...
5 years, 3 months ago (2015-09-03 11:38:42 UTC) #11
Michael Starzinger
LGTM. Just nits and a suggestions. https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode25 src/compiler/bytecode-graph-builder.cc:25: locals_count_(locals_count), nit: Wouldn't ...
5 years, 3 months ago (2015-09-03 12:19:40 UTC) #12
rmcilroy
Looking really good, mostly just nits with one optional suggestion. https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode38 ...
5 years, 3 months ago (2015-09-03 12:41:58 UTC) #13
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48 src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver On 2015/09/03 12:41:57, rmcilroy wrote: > ...
5 years, 3 months ago (2015-09-03 12:52:07 UTC) #14
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48 src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver On 2015/09/03 12:52:07, Michael Starzinger wrote: ...
5 years, 3 months ago (2015-09-03 12:55:19 UTC) #15
oth
Thanks for the comments, definitely moving in the right direction. Unit tests for the bytecode ...
5 years, 3 months ago (2015-09-04 11:06:36 UTC) #16
rmcilroy
I like the iterator :). A few more comments, I'll look in more depth when ...
5 years, 3 months ago (2015-09-04 11:35:37 UTC) #17
oth
Thanks, incorporated. https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode35 src/compiler/bytecode-graph-builder.cc:35: // On 2015/09/04 11:35:37, rmcilroy wrote: > ...
5 years, 3 months ago (2015-09-04 16:15:46 UTC) #18
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h File src/compiler/linkage.h (right): https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h#newcode333 src/compiler/linkage.h:333: static const int kInterpreterReceiverParameter = -1; On 2015/09/04 16:15:46, ...
5 years, 3 months ago (2015-09-04 16:35:51 UTC) #19
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op = common()->Parameter(-1, nullptr); Something is highly ...
5 years, 3 months ago (2015-09-04 16:46:58 UTC) #20
rmcilroy
On 2015/09/04 16:46:58, Michael Starzinger wrote: > https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc > File src/compiler/bytecode-graph-builder.cc (right): > > https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 ...
5 years, 3 months ago (2015-09-07 08:38:35 UTC) #21
oth
Thanks Michael, PTAQL. https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op = common()->Parameter(-1, nullptr); ...
5 years, 3 months ago (2015-09-07 08:40:15 UTC) #22
Michael Starzinger
https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46 src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op = common()->Parameter(-1, nullptr); On 2015/09/07 08:40:15, ...
5 years, 3 months ago (2015-09-07 09:51:31 UTC) #23
rmcilroy
Looks really great. All nits except for the comment about TwoParameterTest, but the rest lgtm! ...
5 years, 3 months ago (2015-09-10 10:19:43 UTC) #24
oth
Thanks! All comments incorporated. Only big deltas are in test-run-bytecode-graph-builder.cc. https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode18 ...
5 years, 3 months ago (2015-09-10 13:59:06 UTC) #25
rmcilroy
Looks great, let's land it! https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc File src/interpreter/bytecode-array-iterator.cc (right): https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67 src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants, GetIndexOperand(operand_index)); On ...
5 years, 3 months ago (2015-09-10 14:06:28 UTC) #26
rmcilroy
Looks great, let's land it! https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc File src/interpreter/bytecode-array-iterator.cc (right): https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67 src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants, GetIndexOperand(operand_index)); On ...
5 years, 3 months ago (2015-09-10 14:06:28 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291693004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291693004/300001
5 years, 3 months ago (2015-09-10 14:19:46 UTC) #29
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/builds/7921)
5 years, 3 months ago (2015-09-10 14:31:41 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291693004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291693004/340001
5 years, 3 months ago (2015-09-10 15:49:00 UTC) #34
commit-bot: I haz the power
Committed patchset #18 (id:340001)
5 years, 3 months ago (2015-09-10 16:21:39 UTC) #35
commit-bot: I haz the power
Patchset 18 (id:??) landed as https://crrev.com/8df7b4f6b502e2c318b61ce604332d51544081c6 Cr-Commit-Position: refs/heads/master@{#30687}
5 years, 3 months ago (2015-09-10 16:21:55 UTC) #36
brucedawson
5 years, 3 months ago (2015-09-14 17:43:05 UTC) #38
Message was sent while issue was closed.
This mornings 'new warning' report from the /analyzer builder pointed out some
variable shadowing. Not bugs, but worth mentioning.

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:392: const Operator* op =
common()->IfSuccess();
This shadows the 'op' function parameter, which makes the code slightly more
confusing. Consider renaming to avoid shadowing?

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:418: Node* inputs[] = {control, other};
This shadows the local 'inputs' variable declared at the top. I'm not sure
whether the types being different makes it more or less confusing.

Consider renaming to avoid shadowing?

Powered by Google App Engine
This is Rietveld 408576698