Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
487017:
Refactor Reference so that SetValue and GetValue pop the reference state. (Closed)
|
Created:
11 years ago by William Hesse Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionRefactor Reference so that SetValue and GetValue pop the reference state.
Committed: http://code.google.com/p/v8/source/detail?r=3720
Patch Set 1 #
Total comments: 2
Patch Set 2 : '' #
Total comments: 10
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 39
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 6
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 15
Patch Set 16 : Refactor Reference class so inline caches can take arguments in registers. #Patch Set 17 : '' #
Total comments: 6
Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #
Messages
Total messages: 20 (0 generated)
Design discussion: If Reference's constructor pushes the state on the stack, and GetValue and SetValue and TakeValue pop the state from the stack, then the destructor can just verify that the state is popped. I assume that since a Reference scope is contained in the compilation of an expression, the only way runtime control flow exits is through generating a JS exception, so the stack is cleaned up. This means the stack height must be cleaned up, regardless of the extra stuff put on by the Reference. Compile-time control flow leaving a Reference scope (because of compile errors? out-of-memory? we are in an expression) will call the Reference destructor, so they must not skip the GetValue and SetValue effects if we are checking the state (number of pushed copies of state remaining) in debug modes. Let me know what you think of this design. The code is not ready for review, of course. http://codereview.chromium.org/487017/diff/1/2 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/1/2#newcode4435 src/x64/codegen-x64.cc:4435: PushFrameSlotAt(element_count() - 2); Oops: frame_->PushFrameSlotAt. But it might be private. http://codereview.chromium.org/487017/diff/1/3 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/1/3#newcode58 src/x64/codegen-x64.h:58: I use this enum argument, rather than a boolean argument. If we needed one call site to call both variants, this wouldn't work.
1. I don't know how well two different constructors will work because you have to conditionally select them. Probably simplest to pass a flag to the single constructor. You'll ideally just have something like this in VisitAssignment: Reference ref(this, node->is_compound_assignment()); ... 2. For now (before ICs are rewritten) we should not regress performance. So, GetValue should (a) work just like before for a 'compound assignment' reference or (b) call the IC and then drop the receiver/key from the stack otherwise. Consider invalidating the reference in case (b). SetValue should only work for references that haven't been invalidated, and should call the IC then drop the receiver/key from the stack (invalidating the reference). The destructor can just check that the reference has been invalidated.
OK, working now except for initialization blocks with SetValue, TakeValue, and GetValue popping the reference appropriately.
Hi Bill, I haven't considered how easy it would be to do, but it seems nicer to never (re)materialize the receiver and possibly key on the frame for non-compound assignment loads, rather than materializing them and then dropping them. In the case of an inlined property load, we can end up (1) emitting code to move a non-live receiver or key value to a register on the slow path (meanwhile increasing register pressure and possibly introducing spills on the fast path) or else (2) materializing a non-live receiver or key value on the stack on the fast path only to drop it on all paths. That means moving the check for compound assignment into the different cases of inlined/non-inlined slot/property/keyed property loads. It might be nice to see how much of a difference it makes (or if my intuition is completely wrong). http://codereview.chromium.org/487017/diff/2003/2004 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/2003/2004#newcode4357 src/x64/codegen-x64.cc:4357: Expression* expression, Tabs used for indentation? http://codereview.chromium.org/487017/diff/2003/2004#newcode4359 src/x64/codegen-x64.cc:4359: : cgen_(cgen), expression_(expression), Our normal style is to put these all on the same line or else one per line. http://codereview.chromium.org/487017/diff/2003/2004#newcode4366 src/x64/codegen-x64.cc:4366: cgen_->UnloadReference(this); No, ASSERT(is_invalid()). Reference should have been unloaded or never loaded (for reference errors). (or is_unloaded() or whatever you call it). http://codereview.chromium.org/487017/diff/2003/2004#newcode4423 src/x64/codegen-x64.cc:4423: if (!ref->is_illegal()) { I'm not sure why this check. The size of the illegal reference is zero, so it seems unnecessary. Moreover I would expect that we would not want to ever call UnloadReference on one that is not loaded. Just ASSERT(!is_invalid()) or whatever you call it. http://codereview.chromium.org/487017/diff/2003/2004#newcode4425 src/x64/codegen-x64.cc:4425: } I expected this to set the type to INVALID or UNLOADED or whatever. You wouldn't want to call UnloadReference without changing the type, would you? http://codereview.chromium.org/487017/diff/2003/2004#newcode6043 src/x64/codegen-x64.cc:6043: type_ = ILLEGAL; No need to change the type if you let UnloadReference do it. http://codereview.chromium.org/487017/diff/2003/2004#newcode6082 src/x64/codegen-x64.cc:6082: if (!is_compound_assignment_) { Why not ASSERT(is_compound_assignment()) instead? http://codereview.chromium.org/487017/diff/2003/2004#newcode6217 src/x64/codegen-x64.cc:6217: type_ = ILLEGAL; Again, could consider letting UnloadReference change the type to simplify the protocol. http://codereview.chromium.org/487017/diff/2003/2005 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/2003/2005#newcode56 src/x64/codegen-x64.h:56: enum Type { ILLEGAL = -1, SLOT = 0, NAMED = 1, KEYED = 2 }; It seems strange that ILLEGAL now means both "illegal left-hand side (throw reference error)" and "already been unloaded". Perhaps "INVALID" (or even UNLOADED or NOT_LOADED) is a better name. http://codereview.chromium.org/487017/diff/2003/2005#newcode59 src/x64/codegen-x64.h:59: Expression* expression, Are these tabs used for indentation?
Added a straightforward fix for initialization blocks. I think I would like to get unloading of references close into the SetValue and GetValue on all platforms in one change, and then get the unloading further in to the inline loads as suggested by Kevin in a later change.
There is a strange stack overflow in mjsunit/array-reduce.js, and an error in arguments-apply.js.
OK, passes tests, seems to have no performance impact, is ready for review on X64 platform.
The X64 change seems ok to me (without understanding it completely). There is no x64-specific code in it. It's all virtual-frame operations. http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode1829 src/x64/codegen-x64.cc:1829: // that it doesn't matter whether a value (eg, ebx pushed above) is ebx->rbx http://codereview.chromium.org/487017/diff/8006/10006#newcode2734 src/x64/codegen-x64.cc:2734: frame_->Push(&receiver); Could this be made faster by doing, effectively: pop(tmp) push(Operand(sp)) // aka Dup. mov(Operand(sp,kPointerSize), tmp) It seems like one less operation - but maybe not if the frame is virtual. http://codereview.chromium.org/487017/diff/8006/10006#newcode2958 src/x64/codegen-x64.cc:2958: frame_->Push(&receiver); Again a swap of the top two elements of the frame. Perhaps give the frame a Swap operation.
ia32 tested, arm almost finished. http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode2734 src/x64/codegen-x64.cc:2734: frame_->Push(&receiver); On 2009/12/17 09:59:02, Lasse Reichstein wrote: > Could this be made faster by doing, effectively: > pop(tmp) > push(Operand(sp)) // aka Dup. > mov(Operand(sp,kPointerSize), tmp) > It seems like one less operation - but maybe not if the frame is virtual. The frame may well be virtual, in which case these may generate little or no runtime code. http://codereview.chromium.org/487017/diff/8006/10006#newcode2958 src/x64/codegen-x64.cc:2958: frame_->Push(&receiver); On 2009/12/17 09:59:02, Lasse Reichstein wrote: > Again a swap of the top two elements of the frame. > Perhaps give the frame a Swap operation. This may be a good idea. I'm not sure how much to optimize the resulting Swap operation, though.
I just commented on the x64 version (from patch set 7). It seems like the right idea, but I'm hoping you can simplify or clarify a few more things. http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode672 src/x64/codegen-x64.cc:672: // Frame[2]: the function Function.prototype.apply, or some other .apply Just the apply property of frame[3]. http://codereview.chromium.org/487017/diff/8006/10006#newcode673 src/x64/codegen-x64.cc:673: // Frame[3]: the function that we fetched .apply on. Not necessarily a function. http://codereview.chromium.org/487017/diff/8006/10006#newcode738 src/x64/codegen-x64.cc:738: frame_->SyncRange(0, frame_->element_count() - 1); I'm not sure this is quite the way to write it. You should either (a) use a SpilledScope so we get (some) debug asserts that we don't have any virtual elements on the frame and so we can see where it ends, or (b) allow the register allocator to work for this code. For instance, I don't think there's any particular reason to spill the entire frame so that you can fetch the function from memory into rdi explicitly. If it was already in some other register, that would be fine for this snippet. http://codereview.chromium.org/487017/diff/8006/10006#newcode807 src/x64/codegen-x64.cc:807: Result a2 = frame_->Pop(); This code snippet seems like it could be streamlined too. There's no need to move the args from memory to a register and then back to memory (for the call). Why not just: Result app = frame_->ElementAt(2); Result fun = frame_->ElementAt(3); frame_->SetElementAt(2, &fun); frame_->SetElementAt(3, &app); http://codereview.chromium.org/487017/diff/8006/10006#newcode824 src/x64/codegen-x64.cc:824: frame_->Nip(1); Is it necessary to Nip (removing values from the middle of the frame can be avoided by not eagerly putting them on)? From invoke: Result result = allocator()->Allocate(rax); ASERT(result.is_valid()); frame_->Drop(); // Discard the function. done.Jump(&result); And here: Result res = frame_->CallStub(&call_function, 3); if (try_lazy) done.Bind(&res); frame_->RestoreContextRegister(); frame_->SetElementAt(0, &res); // Overwrite the duplicate of the receiver of '.apply'. Please be careful about the terminology in comments. The callee is not necessarily a function. http://codereview.chromium.org/487017/diff/8006/10006#newcode1822 src/x64/codegen-x64.cc:1822: // unloading the reference itself (which preserves the top of stack, This comment is misleading. Doesn't SetValue now remove the reference? http://codereview.chromium.org/487017/diff/8006/10006#newcode1826 src/x64/codegen-x64.cc:1826: frame_->Drop(); Why not drop the value and the duplicate? frame_->Drop(2) http://codereview.chromium.org/487017/diff/8006/10006#newcode1831 src/x64/codegen-x64.cc:1831: each.SetValue(NOT_CONST_INIT); And drop the value here frame_->Drop() Even better (you should probably just make it a separate change) is that GetValue and SetValue can return Results instead of communicating them via the frame. Makes the code a little more straightforward. http://codereview.chromium.org/487017/diff/8006/10006#newcode1881 src/x64/codegen-x64.cc:1881: { Reference ref(this, node->catch_var()); No real need to have a Reference here. I'm pretty sure this is always a VariableProxy that is a Slot with type LOCAL. http://codereview.chromium.org/487017/diff/8006/10006#newcode2673 src/x64/codegen-x64.cc:2673: if (target.size() == 1) { if (target.type() == Reference::NAMED) seems clearer. http://codereview.chromium.org/487017/diff/8006/10006#newcode2944 src/x64/codegen-x64.cc:2944: { I don't understand why this block scope is here? The destructor for the reference just asserts it's unloaded and the destructor for the result unuses it but it's unused by the push. http://codereview.chromium.org/487017/diff/8006/10006#newcode2954 src/x64/codegen-x64.cc:2954: { Likewise this block scope. I'm scratching my head trying to figure out why it's here. http://codereview.chromium.org/487017/diff/8006/10006#newcode3349 src/x64/codegen-x64.cc:3349: if (is_postfix) frame_->SetElementAt(is_const ? 0 : target.size(), OK, but it seems awkward. If Reference::size just reports the space currently used by the reference on the frame (ie, illegal and unloaded are both 0), then you can just use the old code. http://codereview.chromium.org/487017/diff/8006/10006#newcode4919 src/x64/codegen-x64.cc:4919: { Reference shadow_ref(this, scope_->arguments_shadow()); It's simplest to just avoid the Reference type completely here (you already don't use it for getting). Slot* shadow = scope_->arguments_shadow()->var()->slot(); Slot* arguments = scope_->arguments()->var()->slot(); ... if (!skip_arguments) { StoreToSlot(arguments, NOT_CONST_INIT); if (mode == LAZY_ARGUMENTS_ALLOCATION) done.Bind(); } StoreToSlot(shadow, NOT_CONST_INIT); http://codereview.chromium.org/487017/diff/8006/10007 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/8006/10007#newcode47 src/x64/codegen-x64.h:47: // reference on the execution stack. The reference is consumed Not execution stack, virtual frame. Not "is", "can be". http://codereview.chromium.org/487017/diff/8006/10007#newcode50 src/x64/codegen-x64.h:50: // it has been consumed, and is in state UNLOADED. For variables Not necessarily in state UNLOADED (or consumed, actually, since ILLEGAL doesn't get consumed). Asserts that it is not on the virtual frame. Talk of variables is misleading because global variables are treated as properties (and so are references to arguments in functions that use the arguments object). http://codereview.chromium.org/487017/diff/8006/10007#newcode80 src/x64/codegen-x64.h:80: ASSERT(!is_unloaded()); Why shouldn't size() be called for unloaded references? It seems like their size is 0.
http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode672 src/x64/codegen-x64.cc:672: // Frame[2]: the function Function.prototype.apply, or some other .apply On 2009/12/18 11:42:54, Kevin Millikin wrote: > Just the apply property of frame[3]. All of these comments rewritten. http://codereview.chromium.org/487017/diff/8006/10006#newcode673 src/x64/codegen-x64.cc:673: // Frame[3]: the function that we fetched .apply on. On 2009/12/18 11:42:54, Kevin Millikin wrote: > Not necessarily a function. Right. This is now called "applicand" http://codereview.chromium.org/487017/diff/8006/10006#newcode738 src/x64/codegen-x64.cc:738: frame_->SyncRange(0, frame_->element_count() - 1); On 2009/12/18 11:42:54, Kevin Millikin wrote: > I'm not sure this is quite the way to write it. You should either (a) use a > SpilledScope so we get (some) debug asserts that we don't have any virtual > elements on the frame and so we can see where it ends, or (b) allow the register > allocator to work for this code. > > For instance, I don't think there's any particular reason to spill the entire > frame so that you can fetch the function from memory into rdi explicitly. If it > was already in some other register, that would be fine for this snippet. This entire sequence always ends in a function call, and it would be much clearer if it was in a spilled scope, starting before LoadAndSpill(receiver). This seems like a change that should be done separately from the change to Reference. I could leave this reference code as is, and clean it up with a later change to TryApplyLazy, or I could fix TryApplyLazy first. This definitely needs a rewrite, with a spilled frame, I think. http://codereview.chromium.org/487017/diff/8006/10006#newcode807 src/x64/codegen-x64.cc:807: Result a2 = frame_->Pop(); On 2009/12/18 11:42:54, Kevin Millikin wrote: > This code snippet seems like it could be streamlined too. There's no need to > move the args from memory to a register and then back to memory (for the call). > Why not just: > > Result app = frame_->ElementAt(2); > Result fun = frame_->ElementAt(3); > frame_->SetElementAt(2, &fun); > frame_->SetElementAt(3, &app); I'm not even sure why we are loading them in the wrong order to begin with. In the fast case, we don't use the .apply function at Frame[2], and in the slow case, we always want to call Frame[3].Frame[2](Frame[1], Frame[0]), which has 3 and 2 in the wrong order. I would like to do this as a separate fix, perhaps first. http://codereview.chromium.org/487017/diff/8006/10006#newcode824 src/x64/codegen-x64.cc:824: frame_->Nip(1); On 2009/12/18 11:42:54, Kevin Millikin wrote: > Is it necessary to Nip (removing values from the middle of the frame can be > avoided by not eagerly putting them on)? > I see what you are doing here, passing the Result rather than putting it on the stack, and it looks better. Can we use this if we have a spilled frame, or should we do it more directly in that case? > From invoke: > > Result result = allocator()->Allocate(rax); > ASERT(result.is_valid()); > frame_->Drop(); // Discard the function. > done.Jump(&result); > > And here: > > Result res = frame_->CallStub(&call_function, 3); > if (try_lazy) done.Bind(&res); > frame_->RestoreContextRegister(); > frame_->SetElementAt(0, &res); // Overwrite the duplicate of the receiver of > '.apply'. > > Please be careful about the terminology in comments. The callee is not > necessarily a function. http://codereview.chromium.org/487017/diff/8006/10007 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/8006/10007#newcode47 src/x64/codegen-x64.h:47: // reference on the execution stack. The reference is consumed On 2009/12/18 11:42:54, Kevin Millikin wrote: > Not execution stack, virtual frame. Not "is", "can be". Done. http://codereview.chromium.org/487017/diff/8006/10007#newcode50 src/x64/codegen-x64.h:50: // it has been consumed, and is in state UNLOADED. For variables On 2009/12/18 11:42:54, Kevin Millikin wrote: > Not necessarily in state UNLOADED (or consumed, actually, since ILLEGAL doesn't > get consumed). Asserts that it is not on the virtual frame. > > Talk of variables is misleading because global variables are treated as > properties (and so are references to arguments in functions that use the > arguments object). Done. http://codereview.chromium.org/487017/diff/8006/10007#newcode80 src/x64/codegen-x64.h:80: ASSERT(!is_unloaded()); On 2009/12/18 11:42:54, Kevin Millikin wrote: > Why shouldn't size() be called for unloaded references? It seems like their > size is 0. Done.
Please look at CallApplyLazy in codegen-x64.cc.
http://codereview.chromium.org/487017/diff/18001/18002 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/18001/18002#newcode664 src/x64/codegen-x64.cc:664: Reference ref(this, apply); I still think you should get rid of this use of Reference. There is no reason for it. Pass the property's object expression to CallApplyLazy instead of the property itself (you've already established that the key is the literal string "apply"), and you can write (for Expression* object): Load(object); Handle<String> name = Factory::LookupAsciiSymbol("apply"); frame()->Push(name); Result answer = frame()->CallLoadIC(RelocInfo::CODE_TARGET); __ nop(); frame()->Push(&answer); And you've evaluated the object and looked up its apply property. No need for Dup and shuffling the extra copy of the object out from under the result (implicitly via the Reference and GetValue). I wouldn't worry about optimizing for an inobject property load in this case, but if you are, you can abstract out the get-named-property code from Reference::GetValue (it should be lifted out anyway) and call that. http://codereview.chromium.org/487017/diff/18001/18002#newcode688 src/x64/codegen-x64.cc:688: Result probe = frame_->Pop(); I would just spill the frame here right after the Pop, and leave the entire rest of the function spilled. http://codereview.chromium.org/487017/diff/18001/18002#newcode785 src/x64/codegen-x64.cc:785: if (try_lazy) { // Separate if (try_lazy) body, without spilled scope. I would eliminate this second test just to get out of the scope. Use a block instead.
http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode1822 src/x64/codegen-x64.cc:1822: // unloading the reference itself (which preserves the top of stack, On 2009/12/18 11:42:54, Kevin Millikin wrote: > This comment is misleading. Doesn't SetValue now remove the reference? Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1826 src/x64/codegen-x64.cc:1826: frame_->Drop(); On 2009/12/18 11:42:54, Kevin Millikin wrote: > Why not drop the value and the duplicate? > > frame_->Drop(2) Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1829 src/x64/codegen-x64.cc:1829: // that it doesn't matter whether a value (eg, ebx pushed above) is On 2009/12/17 09:59:02, Lasse Reichstein wrote: > ebx->rbx Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1831 src/x64/codegen-x64.cc:1831: each.SetValue(NOT_CONST_INIT); On 2009/12/18 11:42:54, Kevin Millikin wrote: > And drop the value here > > frame_->Drop() > > Even better (you should probably just make it a separate change) is that > GetValue and SetValue can return Results instead of communicating them via the > frame. Makes the code a little more straightforward. Will be done in a separate change. http://codereview.chromium.org/487017/diff/8006/10006#newcode1881 src/x64/codegen-x64.cc:1881: { Reference ref(this, node->catch_var()); On 2009/12/18 11:42:54, Kevin Millikin wrote: > No real need to have a Reference here. I'm pretty sure this is always a > VariableProxy that is a Slot with type LOCAL. Left unchanged. http://codereview.chromium.org/487017/diff/8006/10006#newcode2673 src/x64/codegen-x64.cc:2673: if (target.size() == 1) { On 2009/12/18 11:42:54, Kevin Millikin wrote: > if (target.type() == Reference::NAMED) seems clearer. Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode2734 src/x64/codegen-x64.cc:2734: frame_->Push(&receiver); On 2009/12/17 09:59:02, Lasse Reichstein wrote: > Could this be made faster by doing, effectively: > pop(tmp) > push(Operand(sp)) // aka Dup. > mov(Operand(sp,kPointerSize), tmp) > It seems like one less operation - but maybe not if the frame is virtual. I think the frame may well be virtual, and it is clearer the way it is currently written, I think. http://codereview.chromium.org/487017/diff/8006/10006#newcode2958 src/x64/codegen-x64.cc:2958: frame_->Push(&receiver); On 2009/12/18 08:16:55, William Hesse wrote: > On 2009/12/17 09:59:02, Lasse Reichstein wrote: > > Again a swap of the top two elements of the frame. > > Perhaps give the frame a Swap operation. > > This may be a good idea. I'm not sure how much to optimize the resulting Swap > operation, though. > The real problem is that calling x.foo(a,b,c) needs to evaluate x, load x.foo, then call x.foo( x, a, b,c). So x is needed to load x.foo, but also needed on the stack above x.foo, to the the "this" argument to the call. http://codereview.chromium.org/487017/diff/8006/10006#newcode3349 src/x64/codegen-x64.cc:3349: if (is_postfix) frame_->SetElementAt(is_const ? 0 : target.size(), On 2009/12/18 11:42:54, Kevin Millikin wrote: > OK, but it seems awkward. If Reference::size just reports the space currently > used by the reference on the frame (ie, illegal and unloaded are both 0), then > you can just use the old code. Done.
OK, the x64 and ia32 changes are in their final state, unless you find more things to comment on.
I didn't look at the arm code. It looks about right. My comments go for both platforms. http://codereview.chromium.org/487017/diff/28004/20015 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/487017/diff/28004/20015#newcode2357 src/ia32/codegen-ia32.cc:2357: // Frame[3]: applicand. The period looks funny. http://codereview.chromium.org/487017/diff/28004/20015#newcode4905 src/ia32/codegen-ia32.cc:4905: Reference ref(this, property, false); False is the default value, isn't it? http://codereview.chromium.org/487017/diff/28004/20015#newcode4910 src/ia32/codegen-ia32.cc:4910: Reference ref(this, property, false); Do you really need all this? If you just cut out all the keyed load code from Reference::GetValue except the final VirtualFrame::Push, and you call it GenerateKeyedLoad or EmitKeyedLoad and return the Result, you can do: Load(property->obj()); Load(property->key()); Result function = EmitKeyedLoad(); frame()->Drop(); // Key. Result receiver = frame()->Pop(); frame()->Push(&function); frame()->Push(&receiver); http://codereview.chromium.org/487017/diff/28004/20016 File src/ia32/codegen-ia32.h (right): http://codereview.chromium.org/487017/diff/28004/20016#newcode52 src/ia32/codegen-ia32.h:52: // no dynamic state, and uses zero frame slots. Note that some variables There's too much irrelevant detail in this comment. I don't like the "Note that..." part of this comment because it will be out of date as soon as we change VariableProxy rewrites, at which point it will be doing more harm than good. http://codereview.chromium.org/487017/diff/28004/20016#newcode54 src/ia32/codegen-ia32.h:54: // A property whose name is known at compile time uses one frame slot, This part isn't quite right. a["1"] will use two frame slots, for instance. You can say "A property with key()->IsPropertyName() will use one...." or some such. http://codereview.chromium.org/487017/diff/28004/20016#newcode56 src/ia32/codegen-ia32.h:56: // An indexed property, whose name is given by an expression, This isn't really true either (in fact, the key is always "given by an expression" as it has type Expression*). http://codereview.chromium.org/487017/diff/28004/20016#newcode65 src/ia32/codegen-ia32.h:65: bool is_compound_assignment = false); You might consider changing this name to something less (today's) implementation-specific. Knowing us, we'll use the true value for something other than compound assignments somewhere and it'll just be confusing. How about something that has to do with what it means? "should_preserve" or "is_persistent" or something? http://codereview.chromium.org/487017/diff/28004/20016#newcode82 src/ia32/codegen-ia32.h:82: return (type_ == ILLEGAL || type_ == UNLOADED) ? 0 : type_; This already relies on the order of enumeration values. You could write: return (type < SLOT) ? 0 : type_; or even return Max(0, type_); http://codereview.chromium.org/487017/diff/28004/20016#newcode501 src/ia32/codegen-ia32.h:501: // x.apply(y, arguments). We call x the applicand and y the receiver. I wouldn't describe (here) so much detail about how the function is implemented because that could change and isn't important to the interface. You might just indicate that we're trying to avoid allocating the arguments object in that case.
Complete, and finally done. Please suggest any changes you want, or sign off on it. Whew. http://codereview.chromium.org/487017/diff/18001/18002 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/18001/18002#newcode664 src/x64/codegen-x64.cc:664: Reference ref(this, apply); On 2010/01/13 10:40:28, Kevin Millikin wrote: > I still think you should get rid of this use of Reference. There is no reason > for it. Pass the property's object expression to CallApplyLazy instead of the > property itself (you've already established that the key is the literal string > "apply"), and you can write (for Expression* object): > > Load(object); > Handle<String> name = Factory::LookupAsciiSymbol("apply"); > frame()->Push(name); > Result answer = frame()->CallLoadIC(RelocInfo::CODE_TARGET); > __ nop(); > frame()->Push(&answer); > > And you've evaluated the object and looked up its apply property. > > No need for Dup and shuffling the extra copy of the object out from under the > result (implicitly via the Reference and GetValue). > > I wouldn't worry about optimizing for an inobject property load in this case, > but if you are, you can abstract out the get-named-property code from > Reference::GetValue (it should be lifted out anyway) and call that. Done. http://codereview.chromium.org/487017/diff/18001/18002#newcode688 src/x64/codegen-x64.cc:688: Result probe = frame_->Pop(); On 2010/01/13 10:40:28, Kevin Millikin wrote: > I would just spill the frame here right after the Pop, and leave the entire rest > of the function spilled. Done. http://codereview.chromium.org/487017/diff/18001/18002#newcode785 src/x64/codegen-x64.cc:785: if (try_lazy) { // Separate if (try_lazy) body, without spilled scope. On 2010/01/13 10:40:28, Kevin Millikin wrote: > I would eliminate this second test just to get out of the scope. Use a block > instead. Done. http://codereview.chromium.org/487017/diff/28004/20016 File src/ia32/codegen-ia32.h (right): http://codereview.chromium.org/487017/diff/28004/20016#newcode52 src/ia32/codegen-ia32.h:52: // no dynamic state, and uses zero frame slots. Note that some variables On 2010/01/14 16:50:55, Kevin Millikin wrote: > There's too much irrelevant detail in this comment. > > I don't like the "Note that..." part of this comment because it will be out of > date as soon as we change VariableProxy rewrites, at which point it will be > doing more harm than good. Done. http://codereview.chromium.org/487017/diff/28004/20016#newcode54 src/ia32/codegen-ia32.h:54: // A property whose name is known at compile time uses one frame slot, On 2010/01/14 16:50:55, Kevin Millikin wrote: > This part isn't quite right. a["1"] will use two frame slots, for instance. > You can say "A property with key()->IsPropertyName() will use one...." or some > such. Done. http://codereview.chromium.org/487017/diff/28004/20016#newcode56 src/ia32/codegen-ia32.h:56: // An indexed property, whose name is given by an expression, On 2010/01/14 16:50:55, Kevin Millikin wrote: > This isn't really true either (in fact, the key is always "given by an > expression" as it has type Expression*). Done. http://codereview.chromium.org/487017/diff/28004/20016#newcode65 src/ia32/codegen-ia32.h:65: bool is_compound_assignment = false); On 2010/01/14 16:50:55, Kevin Millikin wrote: > You might consider changing this name to something less (today's) > implementation-specific. Knowing us, we'll use the true value for something > other than compound assignments somewhere and it'll just be confusing. > > How about something that has to do with what it means? "should_preserve" or > "is_persistent" or something? Changed to "persist_after_get". http://codereview.chromium.org/487017/diff/28004/20016#newcode82 src/ia32/codegen-ia32.h:82: return (type_ == ILLEGAL || type_ == UNLOADED) ? 0 : type_; On 2010/01/14 16:50:55, Kevin Millikin wrote: > This already relies on the order of enumeration values. You could write: > > return (type < SLOT) ? 0 : type_; > > or even > > return Max(0, type_); Done. http://codereview.chromium.org/487017/diff/28004/20016#newcode501 src/ia32/codegen-ia32.h:501: // x.apply(y, arguments). We call x the applicand and y the receiver. On 2010/01/14 16:50:55, Kevin Millikin wrote: > I wouldn't describe (here) so much detail about how the function is implemented > because that could change and isn't important to the interface. > > You might just indicate that we're trying to avoid allocating the arguments > object in that case. Done.
I'd still like you to get rid of the inaccurate comment before the Reference class. To me, it's worse than no comment at all. The implementation is simple enough that it can speak for itself. http://codereview.chromium.org/487017/diff/28009/20025 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/487017/diff/28009/20025#newcode3094 src/arm/codegen-arm.cc:3094: if (property->is_synthetic()) { I think this is more straightforward. The only difference with synthetic properties is what we use for the receiver. LoadAndSpill(property->obj()); LoadAndSpill(property->key()); EmitKeyedLoad(false); frame_->Drop(); // Key. // Put the function below the receiver. if (property->is_synthetic()) { // Use the global receiver. frame_->Drop(); frame_->EmitPush(r0); LoadGlobalReceiver(r0); } else { frame_->EmitPop(r1); frame_->EmitPush(r0); frame_->EmitPush(r1); } http://codereview.chromium.org/487017/diff/28009/20025#newcode4255 src/arm/codegen-arm.cc:4255: Comment cmnt(masm_, "[ Load from keyed Property"); Not sure why you've got a six space indent here. http://codereview.chromium.org/487017/diff/28009/20026 File src/arm/codegen-arm.h (right): http://codereview.chromium.org/487017/diff/28009/20026#newcode51 src/arm/codegen-arm.h:51: // The reference representing a variable (a bare identifier) usually has This comment is still wrong. Just delete it and let the code speak for itself.
http://codereview.chromium.org/487017/diff/28009/20025 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/487017/diff/28009/20025#newcode3094 src/arm/codegen-arm.cc:3094: if (property->is_synthetic()) { On 2010/01/22 09:09:46, Kevin Millikin wrote: > I think this is more straightforward. The only difference with synthetic > properties is what we use for the receiver. > > LoadAndSpill(property->obj()); > LoadAndSpill(property->key()); > EmitKeyedLoad(false); > frame_->Drop(); // Key. > // Put the function below the receiver. > if (property->is_synthetic()) { > // Use the global receiver. > frame_->Drop(); > frame_->EmitPush(r0); > LoadGlobalReceiver(r0); > } else { > frame_->EmitPop(r1); > frame_->EmitPush(r0); > frame_->EmitPush(r1); > } Done. http://codereview.chromium.org/487017/diff/28009/20025#newcode4255 src/arm/codegen-arm.cc:4255: Comment cmnt(masm_, "[ Load from keyed Property"); On 2010/01/22 09:09:46, Kevin Millikin wrote: > Not sure why you've got a six space indent here. Done. http://codereview.chromium.org/487017/diff/28009/20026 File src/arm/codegen-arm.h (right): http://codereview.chromium.org/487017/diff/28009/20026#newcode51 src/arm/codegen-arm.h:51: // The reference representing a variable (a bare identifier) usually has On 2010/01/22 09:09:46, Kevin Millikin wrote: > This comment is still wrong. Just delete it and let the code speak for itself. Done. Changed on all platforms.
OK, updated to latest V8 revision. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
