I haven't looked at the code it generates yet. Lots of formatting suggestions.
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. You can change this comment. http://codereview.chromium.org/13344/diff/1/2#newcode427 Line 427: if (source.is_register() && target.is_register() && I think this entire loop should be moved into the function MergeToMoveRegisters. 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? 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). http://codereview.chromium.org/13344/diff/1/2#newcode508 Line 508: int first_move_blocked = -1; // Dead initializer. I think we have kIllegalIndex. http://codereview.chromium.org/13344/diff/1/2#newcode516 Line 516: continue; 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. http://codereview.chromium.org/13344/diff/1/2#newcode537 Line 537: Use(target->reg()); 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. http://codereview.chromium.org/13344/diff/1/2#newcode542 Line 542: *source = *target; I would rather have source and target not be pointers, and then have elements_[i] = target here. http://codereview.chromium.org/13344/diff/1/3 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/13344/diff/1/3#newcode451 Line 451: // merge the current frame with the expected frame. "merge this frame with the expected frame" http://codereview.chromium.org/13344/diff/1/3#newcode453 Line 453: // and memory to register moves will follow this call. Is that actually a precondition, or just the way we happen to do it? http://codereview.chromium.org/13344/diff/1/3#newcode459 Line 459: void MergeToMoveRegisters(VirtualFrame *expected, Not clear if it's moving from registers, to registers, or both. Try ProcessRegisterToRegisterMoves? http://codereview.chromium.org/13344 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
