Two comments inline. Still LGTM. On Thu, Jan 8, 2009 at 2:25 PM, Kasper Lund <[email protected]> wrote:
> On Thu, Jan 8, 2009 at 2:13 PM, <[email protected]> wrote: > > LGTM. But for consistency, we should make either all or none of the > > frame's call methods responsible for allocating the eax result register. > > > > > > http://codereview.chromium.org/17414/diff/1/4 > > File src/codegen-ia32.cc (right): > > > > http://codereview.chromium.org/17414/diff/1/4#newcode459 > > Line 459: Result temp = allocator_->Allocate(); > > We haven't sorted out what happens when a call to Allocate() fails. It > > can fail if there are too many off-frame register references. > > > > For now, we have asserts that the result is valid after all the calls to > > Allocate to help us catch when it happens. > > I don't think the code changes I've just made makes any difference > here, but I do agree that we should find a way of dealing with it. > Right now, it looks like we don't have that many off-frame register > references. That's the intent, that they're just local and long-lived references live in the frame where they're eligible for merging. The right thing is to probably assert there are no unexpected off-frame references at select places (entry to every visit function, entry to every labeled basic block) and then write the local code assuming we will be able to allocate k registers. > > > > http://codereview.chromium.org/17414/diff/1/4#newcode1358 > > Line 1358: Result result = allocator_->Allocate(eax); > > CallStub should be responsible for allocating the eax result and > > returning it. There are some overloaded ones that do already. > > > > http://codereview.chromium.org/17414/diff/1/4#newcode3198 > > Line 3198: Result result = allocator_->Allocate(eax); > > Ditto CallCodeObject. > > My plan was to do this once all the CallCodeObject and CallStub sites > had been rewritten to deal with register allocation. I hope that's > okay. I already changed CallStub, even though the spilled call sites just ignore the return value. It's OK to clean this one up later. > > > > http://codereview.chromium.org/17414/diff/1/4#newcode3266 > > Line 3266: frame_->EmitPush(frame_->ElementAt(ref.size())); > > There is a function in the frame class that copies a (base-relative) > > frame element to the top of the frame without requiring spilling. Maybe > > it's worth considering the top-relative version of that here even though > > we know a call is coming right up. > > That's certainly the next thing on the list. > > Cheers, > Kasper > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
