Sounds perfect - like what I was thinking. Many uses need the contents of that register, or the new copy, though, so they will need to extract the FrameElement before calling AdjustCopies.
On Thu, Mar 19, 2009 at 4:45 PM, <[email protected]> wrote: > The Use/Unuse protocol in AdjustCopies should be simplified, then it > LGTM. > > I have a couple of other comments that could be saved for another > change. I think we still need more aggressive refactoring---there are > too many places where we end up doing almost the same thing (adjusting > copies, unusing registers, overwriting frame element). > > > http://codereview.chromium.org/50012/diff/1/2 > File src/virtual-frame-ia32.cc (right): > > http://codereview.chromium.org/50012/diff/1/2#newcode525 > Line 525: __ mov(backing_reg, Operand(ebp, fp_relative(index))); > I think it's clearer to Use(backing_reg) just before or after this line > rather than unusing it in the register case and then calling use on it > again in both cases. That way, there is a single use on the memory > case, and no unuse/use on the register case. > > http://codereview.chromium.org/50012/diff/1/2#newcode535 > Line 535: new_backing_element.set_sync(); > You might consider just putting the creation of the new element and > assignment to elements_[i] in this conditional (making it an if/else). > If we're going to test the sync flag, we might as well create it with > the right one in the first place. > > http://codereview.chromium.org/50012/diff/1/3 > File src/virtual-frame.cc (right): > > http://codereview.chromium.org/50012/diff/1/3#newcode388 > Line 388: Unuse(elements_[frame_index].reg()); > It seems like we actually want to make (a suitably renamed) > "AdjustCopies" always invalidate the frame element at the argument index > (unusing the register), and we might be able to simplify this so we > don't need the second test? > > I don't think we ever need the frame slot after AdjustCopies (that's the > point), so call it "InvalidateFrameSlotAt" or something. It's a little > weird to emit code for a frame element marked invalid, so we could go > farther and have SetFrameSlotAt and pass it in invalid element in the > one case where we *actually* want it to be invalidated. What do you > think? > > > http://codereview.chromium.org/50012 > -- We can IMAGINE what is not --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
