Mostly naming and readability comments. But one thing that starts to be apparent to me is that we are missing a way to get the frame element out of a virtual frame, which makes us use these very thick APIs that should really be named LoadFrameSlotAtAndPushToTOS(). Let's discuss a couple ideas on how to solve this in person.
-Ivan http://codereview.chromium.org/11023/diff/201/401 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/11023/diff/201/401#newcode337 Line 337: // stay in the register.) What is the reason to keep the lowest most register reference of a register instead of the top-most? http://codereview.chromium.org/11023/diff/201/401#newcode339 Line 339: FrameElement memory_element; I find the fact that by default FrameElements are created as memory elements confusing. I was looking at the use of memory_element below and wondered where it was setup as a memory element. One solution would be to have a factory for the frame elements instead of constructors, so that you can say VirtualFrame::MemoryElement(). Maybe you have a better idea. http://codereview.chromium.org/11023/diff/201/401#newcode397 Line 397: } Alternatively instead of increasing the use count for this register again, you could define a RawSpillElementAt(i) which does not deal with any use counts. It could make the code a little bit more readable. http://codereview.chromium.org/11023/diff/201/401#newcode595 Line 595: void VirtualFrame::LoadFrameSlotAt(int index) { Shouldn't this be PushFrameSlotAt()? Essentially this is what it does. Maybe I have done too much RISC assembly in my past, but Load to me indicates a load into a register, but here it has a side-effect of also duplicating the value on the top of the stack which is unclear from the function name. http://codereview.chromium.org/11023/diff/201/401#newcode631 Line 631: void VirtualFrame::StoreToFrameSlotAt(int index) { Note the assymetry from the above function? In this case Store... does not pop from the top of the stack. http://codereview.chromium.org/11023/diff/201/401#newcode639 Line 639: // as the frame top. This comment is slightly misleading:and it is misplaced because the code following right after it deals with the indexed slots register use count. At least: "is now" -> "will be" http://codereview.chromium.org/11023/diff/201/402 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/11023/diff/201/402#newcode269 Line 269: void LoadLocalAt(int index) { Same comments as in the .cc file. LoadLocalAt is asymmetric to StoreToLocalAt() and it has a side effect in that it pushes the value that is not clear from the function name. http://codereview.chromium.org/11023/diff/201/402#newcode293 Line 293: void LoadParameterAt(int index) { ditto. http://codereview.chromium.org/11023/diff/201/402#newcode451 Line 451: void LoadFrameSlotAt(int index); ditto. http://codereview.chromium.org/11023 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
