[Interpreter] Save bytecode offset in interpreter stack frames.

[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/025d476cf57ce8695f8e36906e4863b17c981747
Cr-Commit-Position: refs/heads/master@{#32906}

[rmcilroy, 2015-12-08 10:31:58]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, oth@chromium.org

[rmcilroy, 2015-12-08 10:31:58]

Orion, Michi, please take a look. If you are happy with the general approach I
will port to the other architectures.

This moves the bytecode offset and accumulator saving into well defined offsets
in the interpreter frame, rather than the bytecode handler frame, which will
make them easier to restore during deopt (particularly for inlined functions). I
will need to do something similar for the context, but I'll do this separately
since it needs a bit of changes in context register push/pop in order to do this
without unecessarily adding another stack slot in the interpreter frame.

[oth, 2015-12-08 11:16:47]

On 2015/12/08 10:31:58, rmcilroy wrote:
> Orion, Michi, please take a look. If you are happy with the general approach I
> will port to the other architectures.
> 
> This moves the bytecode offset and accumulator saving into well defined
offsets
> in the interpreter frame, rather than the bytecode handler frame, which will
> make them easier to restore during deopt (particularly for inlined functions).
I
> will need to do something similar for the context, but I'll do this separately
> since it needs a bit of changes in context register push/pop in order to do
this
> without unecessarily adding another stack slot in the interpreter frame.

Looks good here. As discussed we need the bytecode offset for exceptions and
this
wipes out that todo.

[titzer, 2015-12-08 16:25:45]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2015-12-08 16:25:45]

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.cc:98: Node* values[] = {value,
graph()->start(), graph()->start()};
These extra inputs shouldn't be necessary in order to run the schedule verifier.
For the graph verifier, putting in start as the control and effect dependencies
isn't a full solution, unfortunately.

[Michael Starzinger, 2015-12-09 16:35:25]

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.cc:98: Node* values[] = {value,
graph()->start(), graph()->start()};
On 2015/12/08 16:25:45, titzer wrote:
> These extra inputs shouldn't be necessary in order to run the schedule
verifier.
> For the graph verifier, putting in start as the control and effect
dependencies
> isn't a full solution, unfortunately.

As discussed offline, this should no longer be needed after you rebase onto
tip-of-tree.

[rmcilroy, 2015-12-09 16:54:45]

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/1512543002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.cc:98: Node* values[] = {value,
graph()->start(), graph()->start()};
On 2015/12/09 16:35:25, Michael Starzinger wrote:
> On 2015/12/08 16:25:45, titzer wrote:
> > These extra inputs shouldn't be necessary in order to run the schedule
> verifier.
> > For the graph verifier, putting in start as the control and effect
> dependencies
> > isn't a full solution, unfortunately.
> 
> As discussed offline, this should no longer be needed after you rebase onto
> tip-of-tree.

Done. PTAL, thanks.

[Michael Starzinger, 2015-12-09 18:16:22]

As discussed offline: I understand the need to spill the bytecode offset in
order to do reasonable stack unwinding during exception handling. No discussion
needed there and the implementation looks good. So feel free to go ahead with
the ports.

I am still not entirely sure how deoptimization for outer frames of code with
inlined calls will work. Hence I don't yet fully understand why the accumulator
needs to be spilled and restored. I would like to understand this further before
we land the spilling of the accumulator piece.

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:20: typedef
compiler::InterpreterAssembler::SaveAccumulatorMode SaveAccumulatorMode;
nit: Would "using compiler::InterpreterAssembler::SaveAccumulatorMode" work?

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1194: __
CallRuntime(Runtime::kInterpreterToBoolean, accumulator,
We assume that this bytecode is safe with the deoptimizer because
Runtime::kInterpreterToBoolean is assumed to never cause a lazy deopt. In that
case I am not sure whether it is really necessary to spill the accumulator for
this runtime call. Not sure about this one.

https://codereview.chromium.org/1512543002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1512543002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:598: __ Push(rax);
nit: Here "Push(Immediate(0))" should do the trick.

[Michael Starzinger, 2015-12-09 18:22:48]

Maybe we can split this CL apart into two pieces. The first piece just spills
the bytecode offset for now and we can get that non-controversial piece landed
immediately. And the accumulator we tackle when we actually have the deoptimizer
in shape to reconstruct those outer frames. Just a thought.

[rmcilroy, 2015-12-10 14:16:00]

Patchset #4 (id:60001) has been deleted

[rmcilroy, 2015-12-10 14:17:12]

On 2015/12/09 18:22:48, Michael Starzinger wrote:
> Maybe we can split this CL apart into two pieces. The first piece just spills
> the bytecode offset for now and we can get that non-controversial piece landed
> immediately. And the accumulator we tackle when we actually have the
deoptimizer
> in shape to reconstruct those outer frames. Just a thought.

As discussed offline, we don't need the accumulator part for Deopt, so I've
removed that from the CL. PTAL

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:20: typedef
compiler::InterpreterAssembler::SaveAccumulatorMode SaveAccumulatorMode;
On 2015/12/09 18:16:22, Michael Starzinger wrote:
> nit: Would "using compiler::InterpreterAssembler::SaveAccumulatorMode" work?

No, that didn't work (can't remember why). Removed anyway.

https://codereview.chromium.org/1512543002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1194: __
CallRuntime(Runtime::kInterpreterToBoolean, accumulator,
On 2015/12/09 18:16:22, Michael Starzinger wrote:
> We assume that this bytecode is safe with the deoptimizer because
> Runtime::kInterpreterToBoolean is assumed to never cause a lazy deopt. In that
> case I am not sure whether it is really necessary to spill the accumulator for
> this runtime call. Not sure about this one.

Removed accumulator spilling (note the accumulator needed to be saved here even
without deopt because it needs to be saved across the call, and without
kSaveAccumulator it would be clobbered (and not spilled by TF automatically
either).

https://codereview.chromium.org/1512543002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1512543002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:598: __ Push(rax);
On 2015/12/09 18:16:22, Michael Starzinger wrote:
> nit: Here "Push(Immediate(0))" should do the trick.

Done.

[Michael Starzinger, 2015-12-10 14:25:24]

LGTM. Thanks! Please adapt CL title and description before landing.

https://codereview.chromium.org/1512543002/diff/80001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1512543002/diff/80001/src/frames.h#newcode180
src/frames.h:180: static const int kRegisterFilePointerFromFp =
Nice! I like it.

[rmcilroy, 2015-12-10 14:41:08]

Description was changed from

==========
[Interpreter] Save bytecode offset and accumulator in interpreter stack frames.

Adds slots for the bytecode offset and accumulator to interpreter stack
frames and saves them on calls, and restores after calls. The
bytecode offset is always saved, but the accumlator is only saved for
bytecodes which require the accumulator to remain unchanged and don't
replace it.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd and
to have control and effect edges in order to verify correctly.

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

to

==========
[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and 
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

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

[rmcilroy, 2015-12-10 14:41:34]

On 2015/12/10 14:25:24, Michael Starzinger wrote:
> LGTM. Thanks! Please adapt CL title and description before landing.

Done, thanks.

https://codereview.chromium.org/1512543002/diff/80001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1512543002/diff/80001/src/frames.h#newcode180
src/frames.h:180: static const int kRegisterFilePointerFromFp =
On 2015/12/10 14:25:24, Michael Starzinger wrote:
> Nice! I like it.

Acknowledged.

[rmcilroy, 2015-12-10 14:41:40]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-12-10 14:41:45]

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

[commit-bot: I haz the power, 2015-12-10 14:49:06]

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

[commit-bot: I haz the power, 2015-12-10 14:49:07]

Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/381)

[rmcilroy, 2015-12-10 15:15:36]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-12-10 15:15:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1512543002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-10 15:15:52]

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

[commit-bot: I haz the power, 2015-12-10 15:44:07]

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

[commit-bot: I haz the power, 2015-12-10 15:44:08]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/13009)

[rmcilroy, 2015-12-11 15:36:10]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-12-11 15:36:23]

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

[commit-bot: I haz the power, 2015-12-11 15:41:27]

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

[commit-bot: I haz the power, 2015-12-11 15:41:27]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/13094)

[rmcilroy, 2015-12-11 15:43:52]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-12-11 15:43:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1512543002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-11 15:44:00]

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

[commit-bot: I haz the power, 2015-12-11 15:47:17]

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

[commit-bot: I haz the power, 2015-12-11 15:47:17]

Try jobs failed on following builders:
  v8_win_compile_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...)
  v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)

[rmcilroy, 2015-12-11 16:10:46]

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

[commit-bot: I haz the power, 2015-12-11 16:10:58]

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

[commit-bot: I haz the power, 2015-12-11 16:40:02]

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

[commit-bot: I haz the power, 2015-12-11 16:40:03]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/13105)

[rmcilroy, 2015-12-16 14:44:52]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-12-16 14:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1512543002/#ps160001
(title: "Fix ia32 and rebase")

[commit-bot: I haz the power, 2015-12-16 14:44:57]

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

[commit-bot: I haz the power, 2015-12-16 15:14:17]

Description was changed from

==========
[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and 
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

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

to

==========
[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and 
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

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

[commit-bot: I haz the power, 2015-12-16 15:14:17]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-12-16 15:14:25]

Description was changed from

==========
[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and 
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

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

to

==========
[Interpreter] Save bytecode offset in interpreter stack frames.

Adds a slot for the bytecode offset to interpreter stack frames and
saves it on calls, and restores after calls.

Also fixes RawMachineAssembler::Return() to call MergeControlToEnd.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/025d476cf57ce8695f8e36906e4863b17c981747
Cr-Commit-Position: refs/heads/master@{#32906}
==========

[commit-bot: I haz the power, 2015-12-16 15:14:27]

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