Rewritten and improved.
http://codereview.chromium.org/99052/diff/1/2 File src/ia32/virtual-frame-ia32.cc (right): http://codereview.chromium.org/99052/diff/1/2#newcode315 Line 315: if (register_locations_[i] == expected->register_locations_[i]) continue; On 2009/04/27 17:41:57, Kevin Millikin wrote: > register_locations_[i] and register_index(from) are used multiple times in the > loop body and don't change. It's clearer to name them with a local variable. Changed to index and use_index, along with algorithm change. http://codereview.chromium.org/99052/diff/1/2#newcode317 Line 317: && expected->elements_[register_locations_[i]].is_register()) { On 2009/04/27 17:41:57, Kevin Millikin wrote: > Is it the case that if is_used(i) then > expected->elements_[register_locations_[i]] must be a register? We have already > done register-to-memory and register-to-invalid moves, and I don't think > anything else is possible. This was true, correct. I have changed the algorithm to iterate over the target registers, so this is no longer true. We haven't done memory-to-register moves yet. http://codereview.chromium.org/99052/diff/1/2#newcode320 Line 320: if (is_used(to) && !to.is(from)) { On 2009/04/27 17:41:57, Kevin Millikin wrote: > Doesn't register_locations_[i] != expected->register_locations_[i] (and the > property that registers are singly referenced in the frame) imply !to.is(from)? > Maybe that should just be changed to an assert if you're worried about it. Done. http://codereview.chromium.org/99052/diff/1/2#newcode328 Line 328: // Terrible thing to do, but necessary: On 2009/04/27 17:41:57, Kevin Millikin wrote: > I agree it's terrible, but is it really necessary? What's really going on is > that there are nested loops---the outer one iterates over all registers in the > current frame, and the inner one is a while loop iterating until a register > reaches its proper frame index. > It's a little confusing to share the same loop for both purposes. It might even > eliminate some work (like the test is_used(i) and initializing from) to add an > explicit inner loop. Nested loop eliminated. Iteration i's postcondition is that the value that should be in register i is in register i if it was previously in a register, but not a copy. http://codereview.chromium.org/99052/diff/1/2#newcode330 Line 330: } else if (!to.is(from)) { On 2009/04/27 17:41:57, Kevin Millikin wrote: > Is it possible for to.is(from)? Done. http://codereview.chromium.org/99052/diff/1/2#newcode350 Line 350: int j = expected->register_locations_[i]; On 2009/04/27 17:41:57, Kevin Millikin wrote: > Since j is not a loop index, choose a descriptive name (eg, frame_index or > something). Done. More changes also made to this loop. http://codereview.chromium.org/99052/diff/1/2#newcode362 Line 362: __ mov(target.reg(), Operand(ebp, fp_relative(i))); On 2009/04/27 14:20:12, William Hesse wrote: > Change i to j Done. http://codereview.chromium.org/99052 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
