Other than the naming issues, I've addressed your comments.
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.) On 2008/12/11 07:59:36, iposva wrote: > What is the reason to keep the lowest most register reference of a register > instead of the top-most? Ease of implementation for now. Once we start tuning on benchmarks, I'm sure we will discover that it's exactly the wrong heuristic. http://codereview.chromium.org/11023/diff/201/401#newcode339 Line 339: FrameElement memory_element; On 2008/12/11 07:59:36, iposva wrote: > 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. Factory methods are probably cleanest. I'll do that as a separate change. http://codereview.chromium.org/11023/diff/201/401#newcode397 Line 397: } On 2008/12/11 07:59:36, iposva wrote: > 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. Done. http://codereview.chromium.org/11023/diff/201/401#newcode639 Line 639: // as the frame top. On 2008/12/11 07:59:36, iposva wrote: > 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" Fixed. 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) { On 2008/12/11 07:59:36, iposva wrote: > 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. I agree about the naming issue. Perhaps "fetch" is better? I will leave it as is for now and we can discuss it in person. http://codereview.chromium.org/11023 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
