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

Issue 2254493002: [interpreter] Use VisitForTest for loop conditions (Closed)

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

Description

[interpreter] Use VisitForTest for loop conditions Changes the control flow builder classes to make use of the BytecodeLabels helper class. BUG=v8:4280 LOG=n Committed: https://crrev.com/e4c67d3f7042eca64c6146673f7717b079f4ff88 Cr-Commit-Position: refs/heads/master@{#38744}

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : fix while #

Patch Set 4 : fix for as well #

Total comments: 8

Patch Set 5 : make bytecode-loop-analysis cope with multiple backedges #

Patch Set 6 : comments #

Patch Set 7 : minor comment clarification #

Patch Set 8 : typo #

Total comments: 6

Patch Set 9 : comment explaining invariants, move found.. to relevant branch #

Patch Set 10 : initialization #

Patch Set 11 : typo #

Patch Set 12 : rebase(line) golden file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -166 lines) Patch
M src/compiler/bytecode-loop-analysis.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M src/compiler/bytecode-loop-analysis.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +26 lines, -5 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +14 lines, -16 lines 0 comments Download
M src/interpreter/bytecode-label.h View 1 2 3 4 5 1 chunk +12 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-label.cc View 1 chunk +9 lines, -1 line 0 comments Download
M src/interpreter/control-flow-builders.h View 1 2 3 4 5 4 chunks +35 lines, -34 lines 0 comments Download
M src/interpreter/control-flow-builders.cc View 1 2 3 4 5 4 chunks +25 lines, -80 lines 0 comments Download
M test/cctest/compiler/test-run-bytecode-graph-builder.cc View 1 2 3 4 2 chunks +26 lines, -1 line 0 comments Download
M test/cctest/interpreter/bytecode_expectations/BasicLoops.golden View 4 chunks +4 lines, -6 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/DoExpression.golden View 2 chunks +2 lines, -3 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ForOf.golden View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +12 lines, -14 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -4 lines 0 comments Download

Messages

Total messages: 47 (32 generated)
klaasb
Have a look at the arm32-perf results (gsutil.upload) for a supposed 470% improvement on Octane-Ignition-DeltaBlue.
4 years, 4 months ago (2016-08-17 11:37:21 UTC) #12
rmcilroy
Looks great overall. One question about how this interacts with bytecode-loop-analysis though. https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc ...
4 years, 4 months ago (2016-08-17 16:20:43 UTC) #13
klaasb
Fixed the bytecode-loop-analysis to allow multiple backedges for loops and added test cases for that. ...
4 years, 4 months ago (2016-08-18 11:40:00 UTC) #14
klaasb
Hi Jaroslav, I modified the bytecode loop analysis to work with multiple backedges generated when ...
4 years, 4 months ago (2016-08-18 11:46:32 UTC) #18
klaasb
4 years, 4 months ago (2016-08-18 11:46:55 UTC) #20
rmcilroy
LGTM assuming Jaro and Michi are happy with the loop analysis changes.
4 years, 4 months ago (2016-08-18 12:33:22 UTC) #23
Jarin
The loop analysis code looks good to me. https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-loop-analysis.cc File src/compiler/bytecode-loop-analysis.cc (right): https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-loop-analysis.cc#newcode42 src/compiler/bytecode-loop-analysis.cc:42: // ...
4 years, 4 months ago (2016-08-18 12:49:51 UTC) #24
Michael Starzinger
LGTM modulo Jaro's comment. Note that I didn't do a though review and basically only ...
4 years, 4 months ago (2016-08-18 13:24:28 UTC) #25
klaasb
Thanks for the comment, see below for a better correctness argument. https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-loop-analysis.cc File src/compiler/bytecode-loop-analysis.cc (right): ...
4 years, 4 months ago (2016-08-18 13:38:02 UTC) #26
klaasb
https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-loop-analysis.cc File src/compiler/bytecode-loop-analysis.cc (right): https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-loop-analysis.cc#newcode20 src/compiler/bytecode-loop-analysis.cc:20: zone_(zone), On 2016/08/18 13:24:28, Michael Starzinger wrote: > nit: ...
4 years, 4 months ago (2016-08-18 13:50:11 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2254493002/200001
4 years, 4 months ago (2016-08-19 08:47:31 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22102)
4 years, 4 months ago (2016-08-19 08:50:21 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2254493002/220001
4 years, 4 months ago (2016-08-19 08:56:23 UTC) #43
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 4 months ago (2016-08-19 09:21:45 UTC) #45
commit-bot: I haz the power
4 years, 4 months ago (2016-08-19 09:22:53 UTC) #47
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/e4c67d3f7042eca64c6146673f7717b079f4ff88
Cr-Commit-Position: refs/heads/master@{#38744}

Powered by Google App Engine
This is Rietveld 408576698