[fullcodegen] Implement operand stack depth tracking.

[fullcodegen] Implement operand stack depth tracking.

This implements a mechanism to track the exact depth of the operand
stack in full-codegen for every sub-expression visitation. So far we
only tracked the depth at statement level, but not at expression level.
With the introduction of do-expressions it will be possible to construct
local control flow (i.e. break, continue and friends) that target labels
at an arbitrary operand stack depth, making this tracking a prerequisite
for full do-expression support.

R=rossberg@chromium.org,jarin@chromium.org
BUG=v8:4755, v8:4488
LOG=n

Committed: https://crrev.com/38915ed71c179276a1368ea8442bf88f66f50a4c
Cr-Commit-Position: refs/heads/master@{#34211}

[Michael Starzinger, 2016-02-18 17:38:20]

This is still missing architecture ports. I would however like to get a first
round of comments before I do the ports. Thanks!

Jaro: PTAL.
Andreas: FYI.

https://codereview.chromium.org/1706283002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1706283002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:636: DEFINE_BOOL(verify_operand_stack_depth, true,
Note that this flag is on by default only to get try-bot coverage. I will turn
it off before landing this CL. Having it on by default causes a bunch of tests
to timeout so leaving it on is not an option without further tweaking.

[Jarin, 2016-02-19 06:52:04]

Looking great so far! Just a couple of questions.

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
src/full-codegen/full-codegen.cc:280: codegen()->OperandStackDepthIncrement(1);
Could not you call codegen()->PushOperand here?

(And similarly DropOperands, PopOperand below?)

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
src/full-codegen/full-codegen.cc:1322: OperandStackDepthDecrement(2);  // Token
and accumulator are on stack.
Nit: are -> were

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/ia32/full-...
File src/full-codegen/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/ia32/full-...
src/full-codegen/ia32/full-codegen-ia32.cc:1998: __ Assert(equal,
kUnexpectedStackDepth);
Hmm, just wondering: is it ok to clobber eax here? When I was doing this, I had
trouble with clobbering eax and (even the flags!). (However, I did emit the
checks a bit more aggressively.)

[Michael Starzinger, 2016-02-19 09:24:04]

Thanks. Addressed comments. Will do ports now.

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
src/full-codegen/full-codegen.cc:280: codegen()->OperandStackDepthIncrement(1);
On 2016/02/19 06:52:04, Jarin wrote:
> Could not you call codegen()->PushOperand here?
> 
> (And similarly DropOperands, PopOperand below?)

Done. Here and below.

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/full-codeg...
src/full-codegen/full-codegen.cc:1322: OperandStackDepthDecrement(2);  // Token
and accumulator are on stack.
On 2016/02/19 06:52:04, Jarin wrote:
> Nit: are -> were

Done.

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/ia32/full-...
File src/full-codegen/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1706283002/diff/1/src/full-codegen/ia32/full-...
src/full-codegen/ia32/full-codegen-ia32.cc:1998: __ Assert(equal,
kUnexpectedStackDepth);
On 2016/02/19 06:52:04, Jarin wrote:
> Hmm, just wondering: is it ok to clobber eax here? When I was doing this, I
had
> trouble with clobbering eax and (even the flags!). (However, I did emit the
> checks a bit more aggressively.)

Acknowledged. As discussed offline: We are currently only emitting the check
before sub-expression visits, where the accumulator must be assumed to be
clobbered anyways. I think it's fine to clobber until we have a need to emit the
check at places that needs to preserve the accumulator.

[rossberg, 2016-02-19 12:21:17]

LGTM, thanks!

[Michael Starzinger, 2016-02-19 18:16:52]

mstarzinger@chromium.org changed reviewers:
+ v8-mips-ports@googlegroups.com

[Michael Starzinger, 2016-02-19 18:16:52]

PTAL. Jaro, this is ready for final review. Ports done for ARM, ARM64, ia32,
MIPS and x64.

v8-mips-ports@: PTAL at MIPS port. Would you be willing to help with the MIPS64
port?
v8-ppc-ports@: FYI as a heads up.

[balazs.kilvady, 2016-02-19 18:57:06]

On 2016/02/19 18:16:52, Michael Starzinger wrote:
> PTAL. Jaro, this is ready for final review. Ports done for ARM, ARM64, ia32,
> MIPS and x64.
> 
> v8-mips-ports@: PTAL at MIPS port. Would you be willing to help with the
MIPS64
> port?
Sure, on Monday we can start the MIPS64 port.

[Jarin, 2016-02-20 08:18:28]

Awesome, lgtm!

https://codereview.chromium.org/1706283002/diff/140001/src/full-codegen/full-...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1706283002/diff/140001/src/full-codegen/full-...
src/full-codegen/full-codegen.cc:501: DCHECK_GE(operand_stack_depth_, count);
Also check count >= 0?

[Michael Starzinger, 2016-02-22 09:10:55]

Addressed comments.

https://codereview.chromium.org/1706283002/diff/140001/src/full-codegen/full-...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1706283002/diff/140001/src/full-codegen/full-...
src/full-codegen/full-codegen.cc:501: DCHECK_GE(operand_stack_depth_, count);
On 2016/02/20 08:18:27, Jarin wrote:
> Also check count >= 0?

Done. Here and above.

[balazs.kilvady, 2016-02-23 12:41:23]

Hi Michael, sorry for late but please find the MIPS64 port at
https://codereview.chromium.org/1719003005/

[Michael Starzinger, 2016-02-23 12:58:30]

On 2016/02/23 12:41:23, balazs.kilvady wrote:
> Hi Michael, sorry for late but please find the MIPS64 port at
> https://codereview.chromium.org/1719003005/

Cool! Many thanks. I imported the CL. Will land now.

[Michael Starzinger, 2016-02-23 13:38:54]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-02-23 13:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1706283002/#ps220001
(title: "Turn off verification.")

[commit-bot: I haz the power, 2016-02-23 13:39:01]

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

[commit-bot: I haz the power, 2016-02-23 13:40:45]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-02-23 13:41:50]

Description was changed from

==========
[fullcodegen] Implement operand stack depth tracking.

This implements a mechanism to track the exact depth of the operand
stack in full-codegen for every sub-expression visitation. So far we
only tracked the depth at statement level, but not at expression level.
With the introduction of do-expressions it will be possible to construct
local control flow (i.e. break, continue and friends) that target labels
at an arbitrary operand stack depth, making this tracking a prerequisite
for full do-expression support.

R=rossberg@chromium.org,jarin@chromium.org
BUG=v8:4755,v8:4488
LOG=n
==========

to

==========
[fullcodegen] Implement operand stack depth tracking.

This implements a mechanism to track the exact depth of the operand
stack in full-codegen for every sub-expression visitation. So far we
only tracked the depth at statement level, but not at expression level.
With the introduction of do-expressions it will be possible to construct
local control flow (i.e. break, continue and friends) that target labels
at an arbitrary operand stack depth, making this tracking a prerequisite
for full do-expression support.

R=rossberg@chromium.org,jarin@chromium.org
BUG=v8:4755,v8:4488
LOG=n

Committed: https://crrev.com/38915ed71c179276a1368ea8442bf88f66f50a4c
Cr-Commit-Position: refs/heads/master@{#34211}
==========

[commit-bot: I haz the power, 2016-02-23 13:41:51]

Patchset 12 (id:??) landed as
https://crrev.com/38915ed71c179276a1368ea8442bf88f66f50a4c
Cr-Commit-Position: refs/heads/master@{#34211}