First set of comments. I have not looked in detail yet at the allocator. I guess this still needs more work, correct?
-Ivan http://codereview.chromium.org/11396/diff/1/6 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/11396/diff/1/6#newcode165 Line 165: (count < min_count || min_count == -1)) { By starting the min_count with max integer, you can avoid the complicated condition. http://codereview.chromium.org/11396/diff/1/6#newcode176 Line 176: for (; i < stack_pointer_ && min_count > 0; i++) { Personal taste: This is really a while loop, and not a counted for loop. http://codereview.chromium.org/11396/diff/1/6#newcode184 Line 184: for (; i < elements_.length() && min_count > 0; i++) { These two loops should be rolled into one. They essentially do the same work and roll the check whether to sync or not into the loop condition. It is very hard to understand what the difference of the two loops is supposed to mean especially in the absence of comments. I would suggest restructuring it as follows. This also entirely avoids the style comment I had above. for (int i = 0; min_count > 0; i++) { ASSERT(i < elements.length()); if (elements_[i].is_register() && elements[i]...) { // Found one instance of the best_register being used. Spill it. SpillElementAt(i); min_count--; } else { if (i > stack_pointer_) { <- I have fixed the off by one error here. // Make sure to materialize elements on the virtual stack in memory. We rely // on the fact that all elements below a value being written to memory have // been flushed to the physical stack. SyncElementAt(i); } } } http://codereview.chromium.org/11396/diff/1/6#newcode190 Line 190: SyncElementAt(i); It looks to me that there is an off by one error in these two dependent loops. The first loop will exit if (i==stack_pointer_) and the code here will call SyncElementAt(i). The condition within SyncElementAt is i<=stack_pointer_, which means that either the stack_pointer_ needs a better abstraction and/or that the two nested loops are too confusing. http://codereview.chromium.org/11396/diff/1/6#newcode391 Line 391: void VirtualFrame::Drop(int count) { On 2008/11/24 13:44:51, William Hesse wrote: > Drop needs to update the register allocator. We will > do this in the destructor of the frame elements. RemoveLast does not delete the elements and thus it does not call the destructor for the frame elements. http://codereview.chromium.org/11396/diff/1/6#newcode428 Line 428: elements_.Add(FrameElement()); I find this style very confusing. I did not notice this earlier on, but this looks like a local method call, but it actually constructs a temporary object, which will be copied withing the Add method. http://codereview.chromium.org/11396/diff/1/6#newcode451 Line 451: Use(reg); On 2008/11/24 13:44:51, William Hesse wrote: > Change Use(reg) to be in the constructor of the > FrameElement(reg, this) and Unuse(reg) to be in its destructor. > As I pointed out above, you cannot rely on the destructors for the BASE_EMBEDDED types to be called when you "expect" them to be called. Especially when it is added to a List. Also you do not necessarily want to Use within the constructor even if the above problem were not there because you loose control of the allocation. An example of this is the function entry code that needs to populate an initial state. http://codereview.chromium.org/11396/diff/1/6#newcode452 Line 452: elements_.Add(FrameElement(reg)); Same comment as above about the constructor call looking like a function call here but "creating" an anonymous object. http://codereview.chromium.org/11396 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
