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

Issue 1688283003: [Interpreter] Implements calls through CallICStub in the interpreter. (Closed)

Created:
4 years, 10 months ago by mythria
Modified:
4 years, 10 months ago
CC:
v8-reviews_googlegroups.com, oth, v8-x87-ports_googlegroups.com, rmcilroy, v8-ppc-ports_googlegroups.com, v8-mips-ports_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[Interpreter] Implements calls through CallICStub in the interpreter. Calls are implemented through CallICStub to collect type feedback. Adds a new builtin called InterpreterPushArgsAndCallIC that pushes the arguments onto stack and calls CallICStub. Also adds two new bytecodes CallIC and CallICWide to indicate calls have to go through CallICStub. MIPS port contributed by balazs.kilvady. BUG=v8:4280, v8:4680 LOG=N Committed: https://crrev.com/20362a2214c11a0f2ea5141b6a79e09458939cec Cr-Commit-Position: refs/heads/master@{#34244}

Patch Set 1 #

Total comments: 28

Patch Set 2 : Addresses comments and fix for failing bots. #

Patch Set 3 : Fixes comments I missed in last patch. #

Total comments: 12

Patch Set 4 : Fixed comments #

Patch Set 5 : Fixes failing tests. #

Patch Set 6 : Port to arm and arm64 #

Patch Set 7 : Rebased the patch. #

Patch Set 8 : Adds tail call support for CallIC. #

Patch Set 9 : Fixed a bug in ia32 after adding tail call support. #

Patch Set 10 : rebase of patch #

Patch Set 11 : updated mjsunit.status #

Patch Set 12 : mips port contributed by balazs.kilvady. #

Total comments: 6

Patch Set 13 : Fixes comments and adds tests to test-bytecode-generator. #

Patch Set 14 : Fixed an error in test-interpreter. #

Patch Set 15 : removes an unused label declaration. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1142 lines, -278 lines) Patch
M src/arm/builtins-arm.cc View 1 2 3 4 5 6 7 8 9 3 chunks +52 lines, -3 lines 0 comments Download
M src/arm/code-stubs-arm.cc View 1 2 3 4 5 6 7 8 9 7 chunks +28 lines, -8 lines 0 comments Download
M src/arm/interface-descriptors-arm.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M src/arm64/builtins-arm64.cc View 1 2 3 4 5 6 7 8 9 3 chunks +64 lines, -17 lines 0 comments Download
M src/arm64/code-stubs-arm64.cc View 1 2 3 4 5 6 7 8 9 8 chunks +28 lines, -8 lines 0 comments Download
M src/arm64/interface-descriptors-arm64.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M src/builtins.h View 1 2 3 4 5 6 7 8 9 3 chunks +13 lines, -0 lines 0 comments Download
M src/builtins.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M src/code-factory.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M src/code-factory.cc View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -1 line 0 comments Download
M src/code-stubs.h View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -1 line 0 comments Download
M src/compiler/bytecode-graph-builder.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/bytecode-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +38 lines, -7 lines 0 comments Download
M src/ia32/builtins-ia32.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +78 lines, -23 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 4 5 6 7 8 9 8 chunks +30 lines, -8 lines 0 comments Download
M src/ia32/interface-descriptors-ia32.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M src/ic/ic-state.h View 1 2 3 4 5 6 2 chunks +5 lines, -2 lines 0 comments Download
M src/interface-descriptors.h View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -1 line 0 comments Download
M src/interpreter/bytecode-array-builder.h View 1 2 3 4 5 6 7 2 chunks +14 lines, -3 lines 0 comments Download
M src/interpreter/bytecode-array-builder.cc View 1 2 3 4 5 6 7 4 chunks +41 lines, -6 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -3 lines 0 comments Download
M src/interpreter/bytecodes.h View 1 2 3 4 5 6 7 1 chunk +11 lines, -5 lines 0 comments Download
M src/interpreter/bytecodes.cc View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
M src/interpreter/interpreter.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M src/interpreter/interpreter.cc View 1 2 3 4 5 6 7 8 9 2 chunks +52 lines, -1 line 0 comments Download
M src/interpreter/interpreter-assembler.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +12 lines, -0 lines 0 comments Download
M src/interpreter/interpreter-assembler.cc View 1 2 3 4 5 6 7 8 9 1 chunk +13 lines, -0 lines 0 comments Download
M src/mips/builtins-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +66 lines, -13 lines 0 comments Download
M src/mips/code-stubs-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +31 lines, -17 lines 0 comments Download
M src/mips/interface-descriptors-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M src/mips64/builtins-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +66 lines, -12 lines 0 comments Download
M src/mips64/code-stubs-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +37 lines, -23 lines 0 comments Download
M src/mips64/interface-descriptors-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M src/x64/builtins-x64.cc View 1 2 3 4 5 6 7 8 9 3 chunks +43 lines, -1 line 0 comments Download
M src/x64/code-stubs-x64.cc View 1 2 3 4 5 6 7 8 9 8 chunks +31 lines, -9 lines 0 comments Download
M src/x64/interface-descriptors-x64.cc View 1 2 3 4 5 6 7 8 9 1 chunk +13 lines, -0 lines 0 comments Download
M test/cctest/cctest.status View 1 2 3 4 5 6 2 chunks +12 lines, -9 lines 0 comments Download
M test/cctest/compiler/test-run-bytecode-graph-builder.cc View 1 2 3 4 5 6 1 chunk +3 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 29 chunks +183 lines, -76 lines 0 comments Download
M test/cctest/interpreter/test-interpreter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +53 lines, -15 lines 0 comments Download
M test/mjsunit/mjsunit.status View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M test/unittests/interpreter/bytecode-array-builder-unittest.cc View 1 2 3 4 5 6 7 1 chunk +8 lines, -4 lines 0 comments Download

Messages

Total messages: 78 (35 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/1
4 years, 10 months ago (2016-02-12 13:30:41 UTC) #2
mythria
I implemented a new buitin to support calls through CallICStub in the interpreter. I implemented ...
4 years, 10 months ago (2016-02-12 13:35:11 UTC) #4
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/10884)
4 years, 10 months ago (2016-02-12 13:56:13 UTC) #6
mythria
+v8-ppc-ports +v8-mips-ports
4 years, 10 months ago (2016-02-12 14:16:47 UTC) #8
rmcilroy
Looks good overrall, couple of suggestions (although Michael knows this area better than me so ...
4 years, 10 months ago (2016-02-12 14:21:04 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/20001
4 years, 10 months ago (2016-02-12 16:10:36 UTC) #12
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/15270)
4 years, 10 months ago (2016-02-12 16:12:58 UTC) #14
mvstanton
Hi Mythri, looking good, I have a few comments. If an extra push/pop somewhere can ...
4 years, 10 months ago (2016-02-15 11:13:46 UTC) #16
rmcilroy
https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc File src/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc#newcode729 src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register = On 2016/02/15 11:13:45, mvstanton wrote: > ...
4 years, 10 months ago (2016-02-15 11:20:50 UTC) #17
mvstanton
https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc File src/ia32/builtins-ia32.cc (right): https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc#newcode729 src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register = On 2016/02/15 11:20:50, rmcilroy wrote: > ...
4 years, 10 months ago (2016-02-15 11:32:04 UTC) #18
mythria
Thanks for the review. I addressed your comments. I am trying to fix one failing ...
4 years, 10 months ago (2016-02-17 11:02:48 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/80001
4 years, 10 months ago (2016-02-17 14:10:07 UTC) #21
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/1511) v8_linux_mips64el_compile_rel on ...
4 years, 10 months ago (2016-02-17 14:11:00 UTC) #23
balazs.kilvady
On 2016/02/12 14:16:47, mythria wrote: > +v8-ppc-ports > +v8-mips-ports We are working on the MIPS ...
4 years, 10 months ago (2016-02-17 16:19:26 UTC) #24
mvstanton
lgtm
4 years, 10 months ago (2016-02-18 00:55:48 UTC) #25
mythria
On 2016/02/17 16:19:26, balazs.kilvady wrote: > On 2016/02/12 14:16:47, mythria wrote: > > +v8-ppc-ports > ...
4 years, 10 months ago (2016-02-18 10:27:25 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/120001
4 years, 10 months ago (2016-02-18 11:22:04 UTC) #28
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_rel/builds/10394)
4 years, 10 months ago (2016-02-18 11:27:08 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/140001
4 years, 10 months ago (2016-02-18 14:46:59 UTC) #32
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_rel/builds/10414)
4 years, 10 months ago (2016-02-18 14:57:02 UTC) #34
balazs.kilvady
On 2016/02/18 10:27:25, mythria wrote: > On 2016/02/17 16:19:26, balazs.kilvady wrote: > > On 2016/02/12 ...
4 years, 10 months ago (2016-02-18 19:27:03 UTC) #35
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/170001
4 years, 10 months ago (2016-02-22 11:58:42 UTC) #37
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_rel/builds/10535)
4 years, 10 months ago (2016-02-22 12:04:04 UTC) #39
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/210001
4 years, 10 months ago (2016-02-22 12:31:55 UTC) #42
rmcilroy
Great stuff, lgtm, thanks. https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-graph-builder.cc File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-graph-builder.cc#newcode1007 src/compiler/bytecode-graph-builder.cc:1007: } nit - move BuildCall(TailCallMode ...
4 years, 10 months ago (2016-02-22 12:53:23 UTC) #43
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-22 12:58:42 UTC) #45
mythria
Thanks for your reviews. The following are the changes since patchset 6: 1.I added support ...
4 years, 10 months ago (2016-02-22 14:51:32 UTC) #46
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/230001
4 years, 10 months ago (2016-02-22 14:52:09 UTC) #48
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14104)
4 years, 10 months ago (2016-02-22 15:07:08 UTC) #50
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/250001
4 years, 10 months ago (2016-02-22 15:35:20 UTC) #52
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-22 15:56:10 UTC) #54
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/250001
4 years, 10 months ago (2016-02-23 09:23:28 UTC) #56
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-23 09:24:49 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/250001
4 years, 10 months ago (2016-02-23 09:29:46 UTC) #61
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11353)
4 years, 10 months ago (2016-02-23 09:33:02 UTC) #63
mythria
Hi Michi, Could you please take a look at compiler changes. This cl adds new ...
4 years, 10 months ago (2016-02-23 09:48:17 UTC) #65
Michael Starzinger
LGTM, sorry for the delay, this one slipped through the cracks.
4 years, 10 months ago (2016-02-24 10:21:46 UTC) #66
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/270001
4 years, 10 months ago (2016-02-24 10:27:42 UTC) #68
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-24 10:52:35 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688283003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688283003/270001
4 years, 10 months ago (2016-02-24 10:53:06 UTC) #73
commit-bot: I haz the power
Committed patchset #15 (id:270001)
4 years, 10 months ago (2016-02-24 11:00:57 UTC) #75
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/20362a2214c11a0f2ea5141b6a79e09458939cec Cr-Commit-Position: refs/heads/master@{#34244}
4 years, 10 months ago (2016-02-24 11:01:36 UTC) #77
mythria
4 years, 10 months ago (2016-02-24 15:14:40 UTC) #78
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/1731253003/ by mythria@chromium.org.

The reason for reverting is: It is not a good idea to call CallICStub from the
builtin. It might be sensitive to the frame structure. Constructing a internal
frame might cause problems. It is much better to inline the code  related to the
type feedback vector into the builtin. .

Powered by Google App Engine
This is Rietveld 408576698