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#newcode457 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#newcode2098 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.cc#newcode552 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#newcode2215 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#newcode336 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#newcode458 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#newcode2099 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/inline-construct.js File test/mjsunit/compiler/inline-construct.js (right): http://codereview.chromium.org/9304001/diff/16001/test/mjsunit/compiler/inline-construct.js#newcode39 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/inline-construct.js#newcode43 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). http://codereview.chromium.org/9304001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
