OK, all changes made.
http://codereview.chromium.org/13344/diff/1/2 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/13344/diff/1/2#newcode423 Line 423: // Then register-to-register moves, not yet implemented. On 2008/12/10 17:27:06, Kevin Millikin wrote: > You can change this comment. Done. http://codereview.chromium.org/13344/diff/1/2#newcode427 Line 427: if (source.is_register() && target.is_register() && On 2008/12/10 17:27:06, Kevin Millikin wrote: > I think this entire loop should be moved into the function MergeToMoveRegisters. Done. It was actually already there, as the main loop that does work. If no work is needed, it efficiently checks that. http://codereview.chromium.org/13344/diff/1/2#newcode434 Line 434: // Finally, constant-to-register and memory-to-register. We do these from Can we move memory-to-register moves into a separate function? It is quite a distinct operation, with a trivial interface (this and expected). http://codereview.chromium.org/13344/diff/1/2#newcode502 Line 502: bool moves_blocked; // Did we fail to make all the moves on this iteration? On 2008/12/10 17:27:06, Kevin Millikin wrote: > Style guide calls for two spaces between the semicolon and the start of the > comment. I prefer names lie "are_moves_blocked" and "should_break_cycles" and > "were_moves_made" to indicate their flaggishness (they read like counts to me > otherwise). Done. http://codereview.chromium.org/13344/diff/1/2#newcode508 Line 508: int first_move_blocked = -1; // Dead initializer. On 2008/12/10 17:27:06, Kevin Millikin wrote: > I think we have kIllegalIndex. Done. http://codereview.chromium.org/13344/diff/1/2#newcode516 Line 516: continue; On 2008/12/10 17:27:06, Kevin Millikin wrote: > The continues are somewhat confusing. I think deeper nesting is better here. > I'll send you my suggestion separately, because Rietveld doesn't like pasted > code here. Done. http://codereview.chromium.org/13344/diff/1/2#newcode537 Line 537: Use(target->reg()); On 2008/12/10 17:27:06, Kevin Millikin wrote: > I've been trying to follow the convention of updating internal state first and > then emitting code to make it true, whenever it's reasonable. That is, you > could move the Use and Unuse to before the mov instruction. Done. http://codereview.chromium.org/13344/diff/1/2#newcode542 Line 542: *source = *target; On 2008/12/10 17:27:06, Kevin Millikin wrote: > I would rather have source and target not be pointers, and then have > elements_[i] = target here. Done. http://codereview.chromium.org/13344/diff/1/3 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/13344/diff/1/3#newcode43 Line 43: explicit Result(Register reg, CodeGenerator* cgen); On 2008/12/11 08:25:52, iposva wrote: > Do you need explicit here? Done. http://codereview.chromium.org/13344/diff/1/3#newcode54 Line 54: if (is_register() && !reg().is(no_reg)) { On 2008/12/11 08:25:52, iposva wrote: > Maybe you should introduce a special type INVALID. The fact that a Result can be > is_register() but return no_reg for the register value is confusing and will > likely lead to edge cases which will not be caught in regular code. Basically > every user of the Result class will have to not only check for is_register() but > also that the register corresponding to this result is not a no_reg. Done. http://codereview.chromium.org/13344/diff/1/3#newcode451 Line 451: // merge the current frame with the expected frame. On 2008/12/10 17:27:06, Kevin Millikin wrote: > "merge this frame with the expected frame" Done. http://codereview.chromium.org/13344/diff/1/3#newcode453 Line 453: // and memory to register moves will follow this call. On 2008/12/10 17:27:06, Kevin Millikin wrote: > Is that actually a precondition, or just the way we happen to do it? It is a precondition, at least the second part, because additional mem-to-reg moves are generated. http://codereview.chromium.org/13344/diff/1/3#newcode456 Line 456: // memory-to-register moves to break cycles. On 2008/12/11 08:25:52, iposva wrote: > This part of the comment is unclear. If the value is in a register what does it > mean to be converted to a memory-to-register move? And even so, is it even > necessary to put the internal implementation of how cycles are broken into the > function description? Done. http://codereview.chromium.org/13344/diff/1/3#newcode459 Line 459: void MergeToMoveRegisters(VirtualFrame *expected, On 2008/12/10 17:27:06, Kevin Millikin wrote: > Not clear if it's moving from registers, to registers, or both. Try > ProcessRegisterToRegisterMoves? MergeMoveRegistersToRegisters? http://codereview.chromium.org/13344/diff/1/3#newcode460 Line 460: int first_register_move); On 2008/12/11 08:25:52, iposva wrote: > Why does the caller of the API have to pass this parameter? Isn't it the task of > this function to figure out where to start? Done. http://codereview.chromium.org/13344 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
