Add handling for deopt and argument adaptor frames.

Add handling for argument adaptor frames to inlining.
- Lazy deopt from inlined calls
- Lazy deopt from inlined calls with parameter mismatch

R=jarin@chromium.org, mstarzinger@chromium.org, mstarzinger@chromium

Committed: https://code.google.com/p/v8/source/detail?r=24008

[sigurds, 2014-09-15 10:57:35]

sigurds@chromium.org changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org

[sigurds, 2014-09-15 10:57:48]

PTAL.

[Jarin, 2014-09-15 13:37:52]

Looks almost good from my side, but I am sure mstarzinger@ will have bunch of
comments.

https://codereview.chromium.org/573703002/diff/20001/src/compiler/code-genera...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/code-genera...
src/compiler/code-generator.cc:330: descriptor->outer_state()->GetTotalSize() +
i));
How about adding descriptor->outer_state()->GetTotalSize() to frame_state_offset
outside the loop?

https://codereview.chromium.org/573703002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector.cc:1043: if (descriptor->type() == JS_FRAME) {
How about introducing FrameStateDescriptor::HasContext method?

https://codereview.chromium.org/573703002/diff/20001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:439: for (Node* node : visitor.copies()) {
Are we allowed to use C++11? I think not yet.

https://codereview.chromium.org/573703002/diff/20001/src/compiler/node-proper...
File src/compiler/node-properties-inl.h (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/node-proper...
src/compiler/node-properties-inl.h:166: Node* frame_state) {
Perhaps an assert here that the node indeed has frame state input?

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

https://codereview.chromium.org/573703002/diff/20001/src/frames.cc#newcode937
src/frames.cc:937: if (LookupCode()->is_turbofanned() &&
!FLAG_turbo_deoptimization) {
This is a bold move. We should try to run with run-tests
--extra-flags="--turbo-deoptimization" and see what breaks with this.

Do not the GetInlineCount and GetFunctions methods below need similar treatment?

https://codereview.chromium.org/573703002/diff/20001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/20001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-inlining.cc:95: "= 12;"
This is a funky line break. Could you make it a bit more readable?

[sigurds, 2014-09-16 08:39:03]

Thanks for the first round of comments!
mstarzinger, could you please take a look at js-inlining.cc?

https://codereview.chromium.org/573703002/diff/20001/src/compiler/code-genera...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/code-genera...
src/compiler/code-generator.cc:330: descriptor->outer_state()->GetTotalSize() +
i));
On 2014/09/15 13:37:51, jarin wrote:
> How about adding descriptor->outer_state()->GetTotalSize() to
frame_state_offset
> outside the loop?

Done.

https://codereview.chromium.org/573703002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector.cc:1043: if (descriptor->type() == JS_FRAME) {
On 2014/09/15 13:37:51, jarin wrote:
> How about introducing FrameStateDescriptor::HasContext method?

Done.

https://codereview.chromium.org/573703002/diff/20001/src/compiler/node-proper...
File src/compiler/node-properties-inl.h (right):

https://codereview.chromium.org/573703002/diff/20001/src/compiler/node-proper...
src/compiler/node-properties-inl.h:166: Node* frame_state) {
On 2014/09/15 13:37:51, jarin wrote:
> Perhaps an assert here that the node indeed has frame state input?

Done.

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

https://codereview.chromium.org/573703002/diff/20001/src/frames.cc#newcode937
src/frames.cc:937: if (LookupCode()->is_turbofanned() &&
!FLAG_turbo_deoptimization) {
On 2014/09/15 13:37:51, jarin wrote:
> This is a bold move. We should try to run with run-tests
> --extra-flags="--turbo-deoptimization" and see what breaks with this.
> 
> Do not the GetInlineCount and GetFunctions methods below need similar
treatment?

Most of the errors I'm getting seem to be missing deopt points; we need to fix
this later.

https://codereview.chromium.org/573703002/diff/20001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/20001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-inlining.cc:95: "= 12;"
On 2014/09/15 13:37:52, jarin wrote:
> This is a funky line break. Could you make it a bit more readable?

Done.

[Michael Starzinger, 2014-09-16 14:12:56]

Just a few remarks. Didn't do a thorough review of the details.

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
src/compiler/common-operator.h:35: enum FrameStateType { JS_FRAME,
ARGUMENTS_ADAPTOR };
nit: This needs some comments I think. How about the following?

// The type of stack frame that a FrameState node represents.
enum FrameStateType {
  JS_FRAME,          // Represents an unoptimized JavaScriptFrame. 
  ARGUMENTS_ADAPTOR  // Represents an ArgumentsAdaptorFrame.
};

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
src/compiler/common-operator.h:57: Unique<JSFunction> jsfunction_;
nit: IMHO we should just use Handle<JSFunction> instead of Unique<JSFunction>
here. Not sure what a Unique buys us here.

For posterity only: Also this will not necessarily be a constant JSFunction in
the long run. At some point we want to support inlining of non-constant
functions when we know they have a constant underlying SharedFunctionInfo. At
that point this will become an input operand instead of a constant parameter.
Correct?

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:311: class JSCallFunctionAccessor {
While such a thing would be nice to have, spreading it all over the place is a
slippery slop. In an ideal world such an accessor might even be generated from a
DSL (I know I am dreaming here). I could be convinced to let this pass if we add
a big fat TODO(turbofan) here that this should be further nailed down and moved
into the right place later.

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:398: info.EnableDeoptimizationSupport();
OMG, we really should try to find a way to do this lazily when we actually
deopt, instead of eagerly throughout the compilation pipeline. Could we also
leave a TODO(turbofan) comment in that regard?

[sigurds, 2014-09-17 10:02:47]

Thanks for the second round of comments!

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
src/compiler/common-operator.h:35: enum FrameStateType { JS_FRAME,
ARGUMENTS_ADAPTOR };
On 2014/09/16 14:12:56, Michael Starzinger wrote:
> nit: This needs some comments I think. How about the following?
> 
> // The type of stack frame that a FrameState node represents.
> enum FrameStateType {
>   JS_FRAME,          // Represents an unoptimized JavaScriptFrame. 
>   ARGUMENTS_ADAPTOR  // Represents an ArgumentsAdaptorFrame.
> };

Done.

https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-oper...
src/compiler/common-operator.h:57: Unique<JSFunction> jsfunction_;
On 2014/09/16 14:12:56, Michael Starzinger wrote:
> nit: IMHO we should just use Handle<JSFunction> instead of Unique<JSFunction>
> here. Not sure what a Unique buys us here.
> 
> For posterity only: Also this will not necessarily be a constant JSFunction in
> the long run. At some point we want to support inlining of non-constant
> functions when we know they have a constant underlying SharedFunctionInfo. At
> that point this will become an input operand instead of a constant parameter.
> Correct?
Yes, absolutely correct. I propose leaving it as-is, and fixing it when we
support inlining based on SharedFunctionInfo without constant closure.

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:311: class JSCallFunctionAccessor {
On 2014/09/16 14:12:56, Michael Starzinger wrote:
> While such a thing would be nice to have, spreading it all over the place is a
> slippery slop. In an ideal world such an accessor might even be generated from
a
> DSL (I know I am dreaming here). I could be convinced to let this pass if we
add
> a big fat TODO(turbofan) here that this should be further nailed down and
moved
> into the right place later.
I absolutely agree. However, without this thing the code that uses it now
becomes cryptic. I am strongly in favor of the big TODO.

https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:398: info.EnableDeoptimizationSupport();
On 2014/09/16 14:12:56, Michael Starzinger wrote:
> OMG, we really should try to find a way to do this lazily when we actually
> deopt, instead of eagerly throughout the compilation pipeline. Could we also
> leave a TODO(turbofan) comment in that regard?

Done.

[Jarin, 2014-09-17 11:23:19]

lgtm

[Michael Starzinger, 2014-09-17 13:31:44]

Almost there ...

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:340: static void AddClosureToFrameState(JSGraph*
jsgraph, Node* frame_state,
nit: This could be turned into a member method in JSInliner, avoiding the need
to pass the JSGraph around.

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:350: static Node*
CreateArgumentsAdaptorFrameState(JSGraph* jsgraph,
nit: This could be turned into a member method in JSInliner, avoiding the need
to pass the JSGraph and the Zone around.

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:449: outer_frame_state =
CreateArgumentsAdaptorFrameState(
This create a new arguments-adaptor FrameState for every inner FrameState. I
think we should be able to share the arguments adaptor (i.e. create only one per
call-site).

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.stat...
test/cctest/cctest.status:84: 'test-debug/DebugEvaluateWithoutStack': [PASS,
NO_VARIANTS],
Please don't re-introduce these exceptions, they shouldn't be needed anymore. If
they are then something is wrong.

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.stat...
test/cctest/cctest.status:95: 'test-run-inlining/*': [PASS, NO_VARIANTS],
If they do the same, then let's just run them on all variants. They should not
be long running.

[sigurds, 2014-09-17 14:05:13]

Thanks for the comments!

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:340: static void AddClosureToFrameState(JSGraph*
jsgraph, Node* frame_state,
On 2014/09/17 13:31:43, Michael Starzinger wrote:
> nit: This could be turned into a member method in JSInliner, avoiding the need
> to pass the JSGraph around.

Done.

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:350: static Node*
CreateArgumentsAdaptorFrameState(JSGraph* jsgraph,
On 2014/09/17 13:31:43, Michael Starzinger wrote:
> nit: This could be turned into a member method in JSInliner, avoiding the need
> to pass the JSGraph and the Zone around.

Done.

https://codereview.chromium.org/573703002/diff/180001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:449: outer_frame_state =
CreateArgumentsAdaptorFrameState(
On 2014/09/17 13:31:43, Michael Starzinger wrote:
> This create a new arguments-adaptor FrameState for every inner FrameState. I
> think we should be able to share the arguments adaptor (i.e. create only one
per
> call-site).

Good catch, thanks!

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.stat...
test/cctest/cctest.status:84: 'test-debug/DebugEvaluateWithoutStack': [PASS,
NO_VARIANTS],
On 2014/09/17 13:31:43, Michael Starzinger wrote:
> Please don't re-introduce these exceptions, they shouldn't be needed anymore.
If
> they are then something is wrong.

Done.

https://codereview.chromium.org/573703002/diff/180001/test/cctest/cctest.stat...
test/cctest/cctest.status:95: 'test-run-inlining/*': [PASS, NO_VARIANTS],
On 2014/09/17 13:31:43, Michael Starzinger wrote:
> If they do the same, then let's just run them on all variants. They should not
> be long running.

Done.

[Michael Starzinger, 2014-09-17 14:11:55]

LGTM (I skipped looking at the code-generator and the instruction-selector).

[sigurds, 2014-09-17 14:41:53]

Committed patchset #11 (id:200001) manually as 24008 (presubmit successful).