All comments adressed. New version uploaded.
http://codereview.chromium.org/13665/diff/15/16 File src/codegen-ia32.cc (right): http://codereview.chromium.org/13665/diff/15/16#newcode1194 Line 1194: left_side_(left_side), right_side_(right_side) { On 2008/12/11 10:24:16, Kevin Millikin wrote: > The style is to put each initializer on its own line. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1205 Line 1205: VirtualFrame* frame_; On 2008/12/10 16:29:30, Kevin Millikin wrote: > The frame is not needed here---the superclass will handle it. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1212 Line 1212: frame_->SpillAll(); On 2008/12/11 10:24:16, Kevin Millikin wrote: > The frame will already be spilled on entry. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1218 Line 1218: // TODO Add in entry and exit frames for deferred code. On 2008/12/10 16:29:30, Kevin Millikin wrote: > This should already be plumbed in through the superclass and the code generator. > Format todos on the branch as "TODO(): " for now, unless we file an issue. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1235 Line 1235: Register comparee_reg = allocator()->Allocate(); On 2008/12/11 10:24:16, Kevin Millikin wrote: > I would call this register "temp", because it is not (yet) the comparee_reg and > it's confusing to have comparee and comparee_reg at the same time (and have them > be different). Done. http://codereview.chromium.org/13665/diff/15/16#newcode1235 Line 1235: Register comparee_reg = allocator()->Allocate(); On 2008/12/11 10:24:16, Kevin Millikin wrote: > I would call this register "temp", because it is not (yet) the comparee_reg and > it's confusing to have comparee and comparee_reg at the same time (and have them > be different). Done. http://codereview.chromium.org/13665/diff/15/16#newcode1235 Line 1235: Register comparee_reg = allocator()->Allocate(); On 2008/12/10 16:29:30, Kevin Millikin wrote: > I still think this whole operation will be common enough to have its own > abstraction, something like Result::ToRegister. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1237 Line 1237: __ Set(comparee_reg, Immediate(temp.handle())); On 2008/12/11 10:24:16, Kevin Millikin wrote: > What's temp? Done. http://codereview.chromium.org/13665/diff/15/16#newcode1238 Line 1238: comparee = Result(comparee_reg, this); // Usage count has net change 0. On 2008/12/11 10:24:16, Kevin Millikin wrote: > Seems wrong. The allocator has incremented the reference count and the > constructor for the Result has too, so the count is now 2. > > We should avoid explicitly calling the constructor for Result in the code > generator itself. Done. http://codereview.chromium.org/13665/diff/15/16#newcode1248 Line 1248: deferred->exit()->Bind(); On 2008/12/11 10:24:16, Kevin Millikin wrote: > At some point, the reference to the result became dead. Probably right after > the cmp instruction. We will have to track registers through the deferred code > and should talk about it in person. Done. http://codereview.chromium.org/13665/diff/215/216#newcode1233 Line 1233: if (comparee.is_constant()) { On 2008/12/11 19:10:31, iposva wrote: > TODOs should have a bug number associated with it. I know this is on a branch > and if you intend to fix it before work on this branch is merged to > bleeding_edge, then it is OK to leave it like this. Done. http://codereview.chromium.org/13665/diff/215/216#newcode1234 Line 1234: // TODO Could be optimized to a jump at compile time. On 2008/12/11 15:29:55, Kevin Millikin wrote: > Maybe this should be more a more general ToRegister? We might eventually have > non-register types other than constants, and we might also want an overloaded > version to move a Result (even a register one) to a specific register like in > SmiComparisonDeferred::Generate just above (the Result left_side_ is moved into > edx, which shouldn't require emitting any code if it is already in edx). ToRegister implemented and used. ToRegister(Register target) not yet implemented, but should be. http://codereview.chromium.org/13665/diff/215/216#newcode1245 Line 1245: deferred->enter()->Branch(not_zero, not_taken); On 2008/12/11 15:29:55, Kevin Millikin wrote: > I think comparee should be unused right before the bind of exit rather than > after. The register will not be live on either path leading to the label. Done. http://codereview.chromium.org/13665/diff/215/217 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/13665/diff/215/217#newcode56 Line 56: type_ = INVALID; On 2008/12/11 15:29:55, Kevin Millikin wrote: > Is it useful that unusing a constant turns it into invalid? It might be more > useful if the semantics is unuse if a register, otherwise do nothing. I think it is useful that unuse turns a result invalid in all cases. If there is a case where we really want to use a constant after we are done with the register case, we can reconsider. http://codereview.chromium.org/13665/diff/215/218 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/13665/diff/215/218#newcode75 Line 75: void ConvertConstantToRegister(); On 2008/12/11 19:10:31, iposva wrote: > How about just calling this function Load() and that function should do the > right thing based on this item's type? Then you could use it as follows: > Function does as you suggest, but is called ToRegister(). We will also implement ToRegister(Register target) which should move a result to a specified register, freeing its previous register if it exists. > Item comparee = frame_->Pop(); > comparee.Load(); > ASSERT(comparee.is_register()); // This ASSERT is unneeded as comparee.reg() > does make sure that reg() is not called on items that are not of register type. > It is left here for demonstration. > __ test(comparee.reg(), Immediate(kSmiTagMask)); > > > If you don't do it this way you will distribute the checks all over the code > generator and they will keep growing when we eventually add memory based items. > Overall this will lead to toit'er code. http://codereview.chromium.org/13665/diff/215/218#newcode77 Line 77: enum Type {REGISTER, CONSTANT, INVALID}; On 2008/12/11 19:10:31, iposva wrote: > In general we list different enum values on separate lines. I know the code > generator is the biggest offender of this rule, but that does not mean we should > propagate that behaviour. Done. http://codereview.chromium.org/13665 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
