Added new patch set. PTAL.
https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/deoptimizer-arm.cc File src/arm/deoptimizer-arm.cc (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/src/arm/deoptimizer-arm.cc#newcode457 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.cc#newcode2098 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#newcode546 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#newcode4823 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#newcode5045 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#newcode5555 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#newcode5562 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#newcode7237 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#newcode340 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/deoptimizer-ia32.cc File src/ia32/deoptimizer-ia32.cc (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/deoptimizer-ia32.cc#newcode552 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-ia32.cc File src/ia32/lithium-ia32.cc (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/src/ia32/lithium-ia32.cc#newcode2215 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#newcode8234 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-x64.cc File src/x64/builtins-x64.cc (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/builtins-x64.cc#newcode336 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-x64.cc File src/x64/deoptimizer-x64.cc (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/src/x64/deoptimizer-x64.cc#newcode458 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.cc#newcode2099 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/compiler/inline-construct.js File test/mjsunit/compiler/inline-construct.js (right): https://chromiumcodereview.appspot.com/9304001/diff/16001/test/mjsunit/compiler/inline-construct.js#newcode39 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/compiler/inline-construct.js#newcode43 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. https://chromiumcodereview.appspot.com/9304001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
