[Interpreter] Introduce IncStub and DecStub.

[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c58f3285815c44312235030facb86fa0eb2cdc98
Cr-Commit-Position: refs/heads/master@{#35720}

[rmcilroy, 2016-04-19 10:51:53]

Patchset #2 (id:20001) has been deleted

[rmcilroy, 2016-04-19 10:52:16]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-04-19 10:52:38]

rmcilroy@chromium.org changed reviewers:
+ bmeurer@chromium.org, epertoso@chromium.org

[rmcilroy, 2016-04-19 10:52:39]

Benedikt / Enrico, PTAL, thanks.

[epertoso, 2016-04-19 13:08:11]

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:1611: void
IncStub::GenerateAssembly(compiler::CodeStubAssembler* assembler) const {
After http://crrev.com/1893383002, you should use src/code-stub-assembler.h.

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:1655: assembler->LoadObjectField(value,
HeapObject::kMapOffset);
LoadMap

[rmcilroy, 2016-04-19 14:31:46]

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:1611: void
IncStub::GenerateAssembly(compiler::CodeStubAssembler* assembler) const {
On 2016/04/19 13:08:11, epertoso wrote:
> After http://crrev.com/1893383002, you should use src/code-stub-assembler.h.

Done.

https://codereview.chromium.org/1901083002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:1655: assembler->LoadObjectField(value,
HeapObject::kMapOffset);
On 2016/04/19 13:08:10, epertoso wrote:
> LoadMap

Done.

[rmcilroy, 2016-04-19 14:32:11]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 14:32:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901083002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901083002/60001

[commit-bot: I haz the power, 2016-04-19 14:35:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 14:35:26]

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_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/5949)

[epertoso, 2016-04-19 15:48:23]

https://codereview.chromium.org/1901083002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:1623: // Input has been converted to a number using ToNumber,
so it must be a
Where does this conversion happen?

[rmcilroy, 2016-04-19 15:50:03]

https://codereview.chromium.org/1901083002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:1623: // Input has been converted to a number using ToNumber,
so it must be a
On 2016/04/19 15:48:23, epertoso wrote:
> Where does this conversion happen?

In BytecodeGenerator::VisitCountOperation. The value always goes via the
CastAccumulatorToNumber bytecode before it is an input to CountOperation.

[Benedikt Meurer, 2016-04-20 03:52:52]

How about moving the ToNumber into Inc/Dec (at the bytecode level already)? That
way you don't need an explicit ToNumber for ++a/--a, so bytecode size decreases.
That would also be in-line with the other number operations (i.e. Subtract,
Multiply, etc.) that do an implicit ToNumber on the input(s) first.

[rmcilroy, 2016-04-20 08:55:19]

On 2016/04/20 03:52:52, Benedikt Meurer wrote:
> How about moving the ToNumber into Inc/Dec (at the bytecode level already)?
That
> way you don't need an explicit ToNumber for ++a/--a, so bytecode size
decreases.
> That would also be in-line with the other number operations (i.e. Subtract,
> Multiply, etc.) that do an implicit ToNumber on the input(s) first.

Yeah, makes sense. We'd still need the separate ToNumber for postfix operations,
but the added implicit ToNumber here shouldn't have too much perf impact on an
already ToNumbered value, and we get the gain on prefix operations.

[rmcilroy, 2016-04-20 14:49:30]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[rmcilroy, 2016-04-20 14:49:37]

On 2016/04/20 08:55:19, rmcilroy wrote:
> On 2016/04/20 03:52:52, Benedikt Meurer wrote:
> > How about moving the ToNumber into Inc/Dec (at the bytecode level already)?
> That
> > way you don't need an explicit ToNumber for ++a/--a, so bytecode size
> decreases.
> > That would also be in-line with the other number operations (i.e. Subtract,
> > Multiply, etc.) that do an implicit ToNumber on the input(s) first.
> 
> Yeah, makes sense. We'd still need the separate ToNumber for postfix
operations,
> but the added implicit ToNumber here shouldn't have too much perf impact on an
> already ToNumbered value, and we get the gain on prefix operations.

It turns out I can't remove the ToNumber bytecode for prefix operations because
when we compile a TurboFan graph using the bytecode we currently use the JSAdd
operator with a constant 1 for the Inc bytecode, so there isn't an implicit
ToNumber on the value before the add (e.g., if the value is a string "String",
we will end up with "String1" instead of NaN). We can't add the ToNumber
operation explicitly because then we would have a deopt point in the middle of
the bytecode.

I've left it in for now, but we could remove it if we implement JsInc / JsDec
operators in TurboFan (which call these stubs in the generic lowering) down the
road.

[commit-bot: I haz the power, 2016-04-20 14:49:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901083002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901083002/80001

[rmcilroy, 2016-04-20 14:49:53]

Description was changed from

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++, and the value is already converted to a number).

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-04-20 15:15:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 15:15:52]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-04-20 16:18:21]

Patchset #5 (id:120001) has been deleted

[Benedikt Meurer, 2016-04-21 11:37:35]

Very nice! LGTM.

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1625: Label start(assembler, 1, start_vars);
You can just use Label start(assembler, &value_var);

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1707: Label start(assembler, 1, start_vars);
You can just use Label start(assembler, &value_var);

[rmcilroy, 2016-04-21 13:55:31]

Enrico, do you have any more comments before I land this?

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1625: Label start(assembler, 1, start_vars);
I'm sure I tried this first and it didn't work, but works this time :). Done.

https://codereview.chromium.org/1901083002/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1707: Label start(assembler, 1, start_vars);
On 2016/04/21 11:37:35, Benedikt Meurer wrote:
> You can just use Label start(assembler, &value_var);

Done.

[rmcilroy, 2016-04-21 13:55:39]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 13:55:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901083002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901083002/140001

[commit-bot: I haz the power, 2016-04-21 13:57:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 13:57:12]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/535)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/4678)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/4659)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13989)
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/6102)

[rmcilroy, 2016-04-21 14:34:33]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 14:34:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901083002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901083002/160001

[commit-bot: I haz the power, 2016-04-21 14:58:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 14:58:28]

Dry run: This issue passed the CQ dry run.

[epertoso, 2016-04-22 09:40:41]

lgtm

[rmcilroy, 2016-04-22 09:48:45]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-04-22 09:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, epertoso@chromium.org
Link to the patchset: https://codereview.chromium.org/1901083002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-22 09:48:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901083002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901083002/200001

[commit-bot: I haz the power, 2016-04-22 10:35:47]

Description was changed from

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-04-22 10:35:48]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-04-22 19:15:18]

Description was changed from

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Introduce IncStub and DecStub.

Adds IncStub and DecStub TurboFan code stubs and hooks them up to the
interpreter's Inc and Dec bytecodes (which are used for count
operations, e.g. i++).

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c58f3285815c44312235030facb86fa0eb2cdc98
Cr-Commit-Position: refs/heads/master@{#35720}
==========

[commit-bot: I haz the power, 2016-04-22 19:15:19]

Patchset 8 (id:??) landed as
https://crrev.com/c58f3285815c44312235030facb86fa0eb2cdc98
Cr-Commit-Position: refs/heads/master@{#35720}