Regarding the fact that we now have multiply-used registers in the frame, we need to deal with making them writable. I have an idea that might work. Rather than making a Result writable, we make the frame slot that will generate the result writable.
If we will need to write to a frame slot, or if we will need the register that we read from a frame slot to be modifyable, we call MakeExclusive( index ), which leaves memory and constant slots alone, and makes sure that register slots are the exclusive use of that register. On Thu, Dec 11, 2008 at 10:17 AM, <[EMAIL PROTECTED]> wrote: > 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 > -- We can IMAGINE what is not --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
