[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo

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

[rmcilroy, 2015-12-16 14:30:46]

rmcilroy@chromium.org changed reviewers:
+ jarin@chromium.org

[rmcilroy, 2015-12-16 14:30:46]

Jarin@ please take a look. With this change (and Orion's in-flight control-flow
bytecode-graph builder CL patched in) Ignition+TF can pass all of the tests in
cctest/test-deoptimization as well as all of the tests with 'deopt' in the
filename in mjsunit/compiler/ except for:
  compiler/deopt-inlined-smi
  compiler/deopt-tonumber-compare
  compiler/lazy-deopt-in-literal
  compiler/try-deopt
All of which fail due to missing support in the bytecode-generator rather than
issues with deopt.

I've not added any explicit tests since I think it makes more sense to enable
these tests to be run with Ignition+TF on the bots (I'll work on another CL to
do this).

Michi / Orion FYI (comments welcome).

[Jarin, 2015-12-17 13:35:09]

Just a bunch of small comments, lgtm otherwise.

It is a shame that we need the InterpreterNotifyDeoptimized builtin, but I can
see that the ordinary NotifyDeoptimized is tailored for full-code. (It is
strange that the deoptimizer entry stub carefully restores the registers from
the frame description and the NotifyDeoptimized builtin immediately calls the
runtime function, which overwrites most of them.)

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1171: if (context == isolate_->heap()->undefined_value()) {
You should not need this case, that is only for crankshaft. If you want you can
CHECK(context != isolate_->heap()->undefined_value()), but I think such a check
would be useless.

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2000: unsigned fixed_size = ComputeFixedSize(function_,
type);
I am a bit confused here. Input frame should be always optimized. When do you
see non-optimized frame here?

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2016: StackFrame::Type type) const {
As far as I can see all callers statically know which frame type they require.
Could we split this into two functions (one for the interpreter, one for the
rest)?

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2919: return height_ + parameter_count + 3;
Could you possibly add a comment, which explains what is in those 3 values?
(Yeah, I know I am sorry for my sloppiness in the case above.)

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1009
src/frames.cc:1009: // based on the BytecodeArray and bytecode offset.
How about DCHECK_EQ(opcode, Translation::INTERPRETED_FRAME), just for
documentation purposes?

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1094
src/frames.cc:1094: if (opcode == Translation::JS_FRAME) {
I think you also have to handle the interpreter frame here, too.

[rmcilroy, 2015-12-17 23:49:53]

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

[rmcilroy, 2015-12-17 23:49:54]

Feedback addressed and ported to ia32, Arm and Arm64.

+mips-ports: could you port to Mips please. You can test with the following
command:
./out/mips.debug/cctest DeoptimizeSimple --trace-deopt --ignition
--ignition-filter=* --turbo

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1171: if (context == isolate_->heap()->undefined_value()) {
On 2015/12/17 13:35:09, Jarin wrote:
> You should not need this case, that is only for crankshaft. If you want you
can
> CHECK(context != isolate_->heap()->undefined_value()), but I think such a
check
> would be useless.

Good point, removed! Also removed the "!compiled_code_->is_turbofanned() &&
HasAlignmentPadding(function)" above since the code should always be
turbofanned.

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2000: unsigned fixed_size = ComputeFixedSize(function_,
type);
On 2015/12/17 13:35:09, Jarin wrote:
> I am a bit confused here. Input frame should be always optimized. When do you
> see non-optimized frame here?

I agree, I always see OPTIMZED here, I was just confused as to why there was a
if (compiled_code_->kind() == Code::OPTIMIZED_FUNCTION) below so I added this.
Is there a case where compiled_code_->kind() might not be OPTIMIZED_FUNCTION?
Can we remove that check below too?

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2016: StackFrame::Type type) const {
On 2015/12/17 13:35:09, Jarin wrote:
> As far as I can see all callers statically know which frame type they require.
> Could we split this into two functions (one for the interpreter, one for the
> rest)?

Done.

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2919: return height_ + parameter_count + 3;
On 2015/12/17 13:35:09, Jarin wrote:
> Could you possibly add a comment, which explains what is in those 3 values?
> (Yeah, I know I am sorry for my sloppiness in the case above.)

Done. Also for kFunction above.

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1009
src/frames.cc:1009: // based on the BytecodeArray and bytecode offset.
On 2015/12/17 13:35:09, Jarin wrote:
> How about DCHECK_EQ(opcode, Translation::INTERPRETED_FRAME), just for
> documentation purposes?

I had this originally, but opcode gets changed in the code above. I've added a
new local so we can do this.

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1094
src/frames.cc:1094: if (opcode == Translation::JS_FRAME) {
On 2015/12/17 13:35:09, Jarin wrote:
> I think you also have to handle the interpreter frame here, too.

Done.

[Jarin, 2015-12-18 08:58:21]

lgtm with a small nit (+ the mips ports, obviously).

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2000: unsigned fixed_size = ComputeFixedSize(function_,
type);
On 2015/12/17 23:49:54, rmcilroy wrote:
> On 2015/12/17 13:35:09, Jarin wrote:
> > I am a bit confused here. Input frame should be always optimized. When do
you
> > see non-optimized frame here?
> 
> I agree, I always see OPTIMZED here, I was just confused as to why there was a
> if (compiled_code_->kind() == Code::OPTIMIZED_FUNCTION) below so I added this.
> Is there a case where compiled_code_->kind() might not be OPTIMIZED_FUNCTION?
> Can we remove that check below too?

I am not completely sure, but I think it is there to exclude hydrogen stubs...

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1009
src/frames.cc:1009: // based on the BytecodeArray and bytecode offset.
On 2015/12/17 23:49:54, rmcilroy wrote:
> On 2015/12/17 13:35:09, Jarin wrote:
> > How about DCHECK_EQ(opcode, Translation::INTERPRETED_FRAME), just for
> > documentation purposes?
> 
> I had this originally, but opcode gets changed in the code above. I've added a
> new local so we can do this.

Thanks (I am actually surprised that your previous version worked - it looks to
me as if it used a wrong opcode).

https://codereview.chromium.org/1528913003/diff/60001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/60001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1072: // If the optimized frame had alignment padding, adjust
the frame pointer
Nit: This comment should be updated, too.

[rmcilroy, 2015-12-18 15:17:49]

Description was changed from

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo
==========

to

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo
==========

[rmcilroy, 2015-12-18 15:18:11]

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2000: unsigned fixed_size = ComputeFixedSize(function_,
type);
On 2015/12/18 08:58:21, Jarin wrote:
> On 2015/12/17 23:49:54, rmcilroy wrote:
> > On 2015/12/17 13:35:09, Jarin wrote:
> > > I am a bit confused here. Input frame should be always optimized. When do
> you
> > > see non-optimized frame here?
> > 
> > I agree, I always see OPTIMZED here, I was just confused as to why there was
a
> > if (compiled_code_->kind() == Code::OPTIMIZED_FUNCTION) below so I added
this.
> > Is there a case where compiled_code_->kind() might not be
OPTIMIZED_FUNCTION?
> > Can we remove that check below too?
> 
> I am not completely sure, but I think it is there to exclude hydrogen stubs...

Ahh that makes sense. I didn't think they could deopt...

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1528913003/diff/20001/src/frames.cc#newcode1009
src/frames.cc:1009: // based on the BytecodeArray and bytecode offset.
On 2015/12/18 08:58:21, Jarin wrote:
> On 2015/12/17 23:49:54, rmcilroy wrote:
> > On 2015/12/17 13:35:09, Jarin wrote:
> > > How about DCHECK_EQ(opcode, Translation::INTERPRETED_FRAME), just for
> > > documentation purposes?
> > 
> > I had this originally, but opcode gets changed in the code above. I've added
a
> > new local so we can do this.
> 
> Thanks (I am actually surprised that your previous version worked - it looks
to
> me as if it used a wrong opcode).

Yeah, I think it was broken and must have always hit the interpreted frame side.

https://codereview.chromium.org/1528913003/diff/60001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1528913003/diff/60001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1072: // If the optimized frame had alignment padding, adjust
the frame pointer
On 2015/12/18 08:58:21, Jarin wrote:
> Nit: This comment should be updated, too.

Done.

[rmcilroy, 2015-12-18 16:07:38]

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

[commit-bot: I haz the power, 2015-12-18 16:07:45]

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

[commit-bot: I haz the power, 2015-12-18 16:36:59]

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

[commit-bot: I haz the power, 2015-12-18 16:37:00]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-12-18 16:37:44]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-12-18 16:37:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1528913003/#ps80001
(title: "Add MIPS port and fix comment")

[commit-bot: I haz the power, 2015-12-18 16:37:54]

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

[commit-bot: I haz the power, 2015-12-18 18:34:24]

Description was changed from

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo
==========

to

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo
==========

[commit-bot: I haz the power, 2015-12-18 18:34:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-18 18:35:11]

Description was changed from

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo
==========

to

==========
[Interpreter] Add basic deoptimization support from TurboFan to Ignition.

Adds support for generating deoptimization translations for interpreter
stack frames, and building interpreter frames for these translations
when a function deopts. Also adds builtins for
InterpreterNotifyDeoptimized which resume the function's continuation at
the correct point in the interpreter after deopt.

MIPS patch contributed by balazs.kilvady@igmtec.com

BUG=v8:4280
LOG=N
TEST=test-deoptimization.cc with --ignition and --turbo

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

[commit-bot: I haz the power, 2015-12-18 18:35:11]

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