I have only commented virtual-frame-ia32.cc so far, but here they are.
http://codereview.chromium.org/13127/diff/1/5 File src/virtual-frame-ia32.cc (left): http://codereview.chromium.org/13127/diff/1/5#oldcode178 Line 178: while (min_count > 0) { There is no increment of i in this loop. http://codereview.chromium.org/13127/diff/1/5#oldcode196 Line 196: Assert that frame's count of result is 0. http://codereview.chromium.org/13127/diff/209/13 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/13127/diff/209/13#newcode184 Line 184: SyncRange(stack_pointer_ + 1, i); I find it tricky to call this a lot with empty or negative ranges that do nothing. Comment? Should SpillElementAt(i) include this SyncRange call, since it is always a precondition? http://codereview.chromium.org/13127/diff/209/13#newcode298 Line 298: // This code path is currently not triggered. UNIMPLEMENTED is Why is it not triggered? What will make it be triggered? Is it because we sync or spill all before merges? Or because we have no registers and constants? http://codereview.chromium.org/13127/diff/209/13#newcode302 Line 302: Register reg = cgen_->allocator()->AllocateWithoutSpilling(); Does AllocateWithoutSpilling increment the allocator's register count for the result? If so, we are double counting when we call Use() later. If not, how does it record that it is used, so it doesn't return it twice? http://codereview.chromium.org/13127/diff/209/13#newcode317 Line 317: } else { // element is a memory element. http://codereview.chromium.org/13127/diff/209/13#newcode328 Line 328: if (target.is_register()) { Comment or assert that target will never be a source register later in the frame. (or is it obvious that we got it from AllocateWithoutSpilling?) http://codereview.chromium.org/13127/diff/209/13#newcode394 Line 394: SyncRange(stack_pointer_ + 1, i); Won't this hit register-to-register moves and things we keep in registers with a write to the stack? Or is this OK, because something needs to be written to that stack slot anyway? In which case, the earlier source elements, which have already been overwritten by the target, now have their sync bit set. Is this a problem with register-register moves that have not been done yet? We are syncing the previous value! http://codereview.chromium.org/13127/diff/209/13#newcode411 Line 411: // Then register-to-register moves, not yet implemented. Does a register moving to itself need any implementation? Could we say that these are currently handled, and allow them? http://codereview.chromium.org/13127 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
