Implement inlining of constructor calls.

Implement inlining of constructor calls.

R=vegorov@chromium.org,kmillikin@chromium.org

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

[Vyacheslav Egorov (Chromium), 2012-01-30 16:44:05]

It seems that CL lacks changes in deoptimizer that should reconstruct JS
construct stub frame between deoptimized inlined constructor and outer function
(this stub is required to take care of return value if it is not JS object).
Special handling is required when inspecting optimized code from debugger (inner
inlined frames are not expected to be constructor) [take a look at how/where
argument adaptor frame reconstruction/inspection is handled].

HGraphBuilder::GenerateIsConstructCall should also be changed.

[Vyacheslav Egorov (Chromium), 2012-01-30 16:49:55]

Another small comment: we need a good test coverage for this, see for example
tests changed/added in  https://chromiumcodereview.appspot.com/9265004

[Michael Starzinger, 2012-02-02 18:19:24]

Added new patch set. Now with deoptimizer and debugger support. Also fixed an
issue in Hydrogen where the environment didn't fit stack layout of full-codegen
during evaluation of arguments.

[Michael Starzinger, 2012-02-09 10:07:08]

Added new patch set. Ported deoptimizer to x64 and ARM.

[Vyacheslav Egorov (Chromium), 2012-02-13 15:01:38]

first round of comments (overall everything seems to be solid).

http://codereview.chromium.org/9304001/diff/16001/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/arm/deoptimizer-arm.cc#...
src/arm/deoptimizer-arm.cc:457: // Arguments adaptor can not be topmost or
bottommost.
not arguments adaptor.

http://codereview.chromium.org/9304001/diff/16001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:2098: LOperand* function =
UseRegister(instr->function());
do we really need a register for this constant?

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode546
src/hydrogen.cc:546: initial_function_state_(this, info, oracle, false, false),
I think we should come up with enum at least for is_construct.

Call-site gets unreadable.

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode4823
src/hydrogen.cc:4823: bool drop_extra,
Consider introducing enums

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode5045
src/hydrogen.cc:5045: // TODO(3168478): refactor to avoid this.
It seems this comment&todo is outdated (see r7601). 

In case of undefined we can jump directly to the false label.

In case of constructor inlining we jump to the true label.

Consider cleaning this up.

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode5555
src/hydrogen.cc:5555: HInstruction* call = PreProcessCall(
Is lazy bailout handled correctly on this call-site? We need to replace
non-object result value with receiver when we bailout lazily here.

The same for stack inspection at this call-site? (e.g. %_IsConstructCall or
debugger). 

Please ensure that these cases are covered by your test case.

[It seems for simplicity we might want to avoid partial constructor inlining
like this in the initial CL version.]

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode5562
src/hydrogen.cc:5562: HHasInstanceTypeAndBranch* typecheck =
Branch construction code seems to be duplicated with another place
(VisitReturnStatement). Can it be shared?

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.cc#newcode7237
src/hydrogen.cc:7237: outer = new(zone) HEnvironment(outer, target, arguments +
1);
I think HEnvironent constructor for the "artificial" environments can be unified
into one if you start passing frame_type into it.

Then this code can also be moved into something like CreateArtificialEnvironment
(or CreateStubEnvironment or smth).

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/9304001/diff/16001/src/hydrogen.h#newcode340
src/hydrogen.h:340: // XXX
XXX?

http://codereview.chromium.org/9304001/diff/16001/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/ia32/deoptimizer-ia32.c...
src/ia32/deoptimizer-ia32.cc:552: // Arguments adaptor can not be topmost or
bottommost.
Comment mentions arguments adaptor.

I wonder if some fixed part of the frame computation can be shared with
arguments adaptor frame computation.

http://codereview.chromium.org/9304001/diff/16001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:2215: LOperand* function =
UseRegister(instr->function());
I wonder if this is needed. We don't use this register inside anyways.

http://codereview.chromium.org/9304001/diff/16001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/objects.cc#newcode8234
src/objects.cc:8234: PrintF(out, "{function=");
It would be still good to see type of the frame in the output.

http://codereview.chromium.org/9304001/diff/16001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:336:
masm->isolate()->heap()->construct_stub_deopt_pc_offset() == 0) {
I don't like dependency on the order of stub generation.

http://codereview.chromium.org/9304001/diff/16001/src/x64/deoptimizer-x64.cc
File src/x64/deoptimizer-x64.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/x64/deoptimizer-x64.cc#...
src/x64/deoptimizer-x64.cc:458: // Arguments adaptor can not be topmost or
bottommost.
not argument adaptor

comment about sharing applies here as well.

http://codereview.chromium.org/9304001/diff/16001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

http://codereview.chromium.org/9304001/diff/16001/src/x64/lithium-x64.cc#newc...
src/x64/lithium-x64.cc:2099: LOperand* function =
UseRegister(instr->function());
I wonder if we really need a register for something that is actually just a
constant.

http://codereview.chromium.org/9304001/diff/16001/test/mjsunit/compiler/inlin...
File test/mjsunit/compiler/inline-construct.js (right):

http://codereview.chromium.org/9304001/diff/16001/test/mjsunit/compiler/inlin...
test/mjsunit/compiler/inline-construct.js:39: return obj;
it might be also interesting to check non-object return value (e.g. number).

http://codereview.chromium.org/9304001/diff/16001/test/mjsunit/compiler/inlin...
test/mjsunit/compiler/inline-construct.js:43: var obj = new c1(a, b);
please extend this test to test inlining in all three contexts of graph
construction (and deopt from them).

[Michael Starzinger, 2012-02-27 14:16:31]

Added new patch set. PTAL.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/deoptimizer...
File src/arm/deoptimizer-arm.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/deoptimizer...
src/arm/deoptimizer-arm.cc:457: // Arguments adaptor can not be topmost or
bottommost.
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> not arguments adaptor.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/lithium-arm...
src/arm/lithium-arm.cc:2098: LOperand* function =
UseRegister(instr->function());
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> do we really need a register for this constant?

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:546: initial_function_state_(this, info, oracle, false, false),
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> I think we should come up with enum at least for is_construct.
> 
> Call-site gets unreadable.

Done. Merged drop_extra and is_construct into one flag.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:4823: bool drop_extra,
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> Consider introducing enums

Done. Merged drop_extra and is_construct into one flag.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:5045: // TODO(3168478): refactor to avoid this.
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> It seems this comment&todo is outdated (see r7601). 
> 
> In case of undefined we can jump directly to the false label.
> 
> In case of constructor inlining we jump to the true label.
> 
> Consider cleaning this up.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:5555: HInstruction* call = PreProcessCall(
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> Is lazy bailout handled correctly on this call-site? We need to replace
> non-object result value with receiver when we bailout lazily here.
> 
> The same for stack inspection at this call-site? (e.g. %_IsConstructCall or
> debugger). 
> 
> Please ensure that these cases are covered by your test case.
> 
> [It seems for simplicity we might want to avoid partial constructor inlining
> like this in the initial CL version.]

Done. I temporarily disabled partial inlining as suggested, by just undoing most
of the changes that have been applied to the hydrogen graph before inlining was
attempted.

It's not pretty and I will try to make partial inlining work with lazy bailout
in a separate CL.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:5562: HHasInstanceTypeAndBranch* typecheck =
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> Branch construction code seems to be duplicated with another place
> (VisitReturnStatement). Can it be shared?

Comment no longer applies after addressing previous comment.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.cc#new...
src/hydrogen.cc:7237: outer = new(zone) HEnvironment(outer, target, arguments +
1);
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> I think HEnvironent constructor for the "artificial" environments can be
unified
> into one if you start passing frame_type into it.
> 
> Then this code can also be moved into something like
CreateArtificialEnvironment
> (or CreateStubEnvironment or smth).

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/hydrogen.h#newc...
src/hydrogen.h:340: // XXX
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> XXX?

Done. Ooops.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/deoptimize...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/deoptimize...
src/ia32/deoptimizer-ia32.cc:552: // Arguments adaptor can not be topmost or
bottommost.
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> Comment mentions arguments adaptor.
> 
> I wonder if some fixed part of the frame computation can be shared with
> arguments adaptor frame computation.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/lithium-ia...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/lithium-ia...
src/ia32/lithium-ia32.cc:2215: LOperand* function =
UseRegister(instr->function());
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> I wonder if this is needed. We don't use this register inside anyways.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/objects.cc#newc...
src/objects.cc:8234: PrintF(out, "{function=");
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> It would be still good to see type of the frame in the output.

You will see the type, because it is printed using
Translation::StringFor(opcode) a few lines above.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/builtins-x6...
File src/x64/builtins-x64.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/builtins-x6...
src/x64/builtins-x64.cc:336:
masm->isolate()->heap()->construct_stub_deopt_pc_offset() == 0) {
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> I don't like dependency on the order of stub generation.

Yep, me neither. I am planning to port the Generate_StringConstructCode() to x64
soon-ish. It's the only architecture still missing (ia32, ARM and MIPS already
have it).

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/deoptimizer...
File src/x64/deoptimizer-x64.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/deoptimizer...
src/x64/deoptimizer-x64.cc:458: // Arguments adaptor can not be topmost or
bottommost.
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> not argument adaptor
> 
> comment about sharing applies here as well.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/lithium-x64...
src/x64/lithium-x64.cc:2099: LOperand* function =
UseRegister(instr->function());
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> I wonder if we really need a register for something that is actually just a
> constant.

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/test/mjsunit/compil...
File test/mjsunit/compiler/inline-construct.js (right):

https://chromiumcodereview.appspot.com/9304001/diff/16001/test/mjsunit/compil...
test/mjsunit/compiler/inline-construct.js:39: return obj;
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> it might be also interesting to check non-object return value (e.g. number).

Done.

https://chromiumcodereview.appspot.com/9304001/diff/16001/test/mjsunit/compil...
test/mjsunit/compiler/inline-construct.js:43: var obj = new c1(a, b);
On 2012/02/13 15:01:39, Vyacheslav Egorov wrote:
> please extend this test to test inlining in all three contexts of graph
> construction (and deopt from them).

Done.

[Vyacheslav Egorov (Chromium), 2012-02-27 15:06:02]

lgtm

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/hydrogen.cc#new...
src/hydrogen.cc:5551: Pop();
I think we can replace receiver with function with SetExpressionStackAt() and
then use PreProcessCall to do heavy lifting with HPushArgument.

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/mips/lithium-co...
File src/mips/lithium-codegen-mips.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/mips/lithium-co...
src/mips/lithium-codegen-mips.cc:4251: __ li(a0, Operand(constructor));
consider using LoadHeapObject/PushHeapObject just to be safe in case if we start
inlining constructors that are in new space (all architectures).

[Michael Starzinger, 2012-02-28 09:07:10]

Addressed comments. Added new patch set. Rebased. Landed.

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/hydrogen.cc#new...
src/hydrogen.cc:5551: Pop();
On 2012/02/27 15:06:02, Vyacheslav Egorov wrote:
> I think we can replace receiver with function with SetExpressionStackAt() and
> then use PreProcessCall to do heavy lifting with HPushArgument.

Done. Made PreProcessCall more templatey.

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/mips/lithium-co...
File src/mips/lithium-codegen-mips.cc (right):

https://chromiumcodereview.appspot.com/9304001/diff/25001/src/mips/lithium-co...
src/mips/lithium-codegen-mips.cc:4251: __ li(a0, Operand(constructor));
On 2012/02/27 15:06:02, Vyacheslav Egorov wrote:
> consider using LoadHeapObject/PushHeapObject just to be safe in case if we
start
> inlining constructors that are in new space (all architectures).

Done (on ARM and MIPS).