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

Issue 2312103002: [Turbofan] Fix CallSuper argument order in BytecodeGraphBuilder. (Closed)

Created:
4 years, 3 months ago by rmcilroy
Modified:
4 years, 3 months ago
Reviewers:
Michael Starzinger
CC:
v8-reviews_googlegroups.com, rmcilroy
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Turbofan] Fix CallSuper argument order in BytecodeGraphBuilder. The constructor and new.target arguments were passed to CallConstruct in the wrong order by BytecodeGraphBuilder, which caused subclassing to be incorrect when optimizing from bytecode. Also clean up some unecessary functions in interpreter.cc found while figuring this out. BUG=chromium:642409 Committed: https://crrev.com/c95025601330103a1caa500c83cad956d71e0ca2 Cr-Commit-Position: refs/heads/master@{#39204}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Shorten test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -56 lines) Patch
M src/compiler/bytecode-graph-builder.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/interpreter/interpreter.h View 2 chunks +0 lines, -15 lines 0 comments Download
M src/interpreter/interpreter.cc View 6 chunks +23 lines, -39 lines 0 comments Download
A test/mjsunit/regress/regress-642409.js View 1 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (8 generated)
rmcilroy
Michi, PTAL, thanks.
4 years, 3 months ago (2016-09-06 11:00:11 UTC) #6
Michael Starzinger
LGTM on the fix, just comments on the regression test. https://codereview.chromium.org/2312103002/diff/1/test/mjsunit/regress/regress-642409.js File test/mjsunit/regress/regress-642409.js (right): https://codereview.chromium.org/2312103002/diff/1/test/mjsunit/regress/regress-642409.js#newcode17 ...
4 years, 3 months ago (2016-09-06 11:11:22 UTC) #7
rmcilroy
https://codereview.chromium.org/2312103002/diff/1/test/mjsunit/regress/regress-642409.js File test/mjsunit/regress/regress-642409.js (right): https://codereview.chromium.org/2312103002/diff/1/test/mjsunit/regress/regress-642409.js#newcode17 test/mjsunit/regress/regress-642409.js:17: for (var i=0; i<50000; i++) { On 2016/09/06 11:11:22, ...
4 years, 3 months ago (2016-09-06 11:27:21 UTC) #8
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/2312103002/20001
4 years, 3 months ago (2016-09-06 11:27:35 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 3 months ago (2016-09-06 11:53:10 UTC) #12
commit-bot: I haz the power
4 years, 3 months ago (2016-09-06 11:53:25 UTC) #14
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/c95025601330103a1caa500c83cad956d71e0ca2
Cr-Commit-Position: refs/heads/master@{#39204}

Powered by Google App Engine
This is Rietveld 408576698