Doesn't seem quite right. We should initially piggyback on the jump targets for the deferred code. I will have a change list ready tomorrow morning to do that, which should simplify things quite a bit here.
Make sure this passes all the V8 tests and Mozilla tests (in both release and debug builds) before submitting, since we don't have a buildbot on the experimental branch. http://codereview.chromium.org/13665/diff/1/3 File src/codegen-ia32.cc (right): http://codereview.chromium.org/13665/diff/1/3#newcode422 Line 422: bool both = true_target.is_linked() && false_target.is_linked(); I think you may need to spill here (or else in VisitBinaryOperation). It seems like you may have a non-spilled frame at the true or false target due to the way we compile short-circuit boolean operators, in which case the EmitPush is not safe. http://codereview.chromium.org/13665/diff/1/3#newcode1184 Line 1184: Register arg_1) There must be a better name. http://codereview.chromium.org/13665/diff/1/3#newcode1197 Line 1197: VirtualFrame* frame_; I think we want to piggy-back on the JumpTarget class---there are actually two frames of interest, the one on entry paired with the entry label, and the one we need to produce on exit paired with the exit label. http://codereview.chromium.org/13665/diff/1/3#newcode1209 Line 1209: // "result" is returned in the flags I don't think this works. There is no guarantee that the frame here after spilling and the call is anything like the frame we expect at the deferred exit label (not necessarily spilled and not even required to be mergable!). http://codereview.chromium.org/13665/diff/1/3#newcode1212 Line 1212: // TODO ??? http://codereview.chromium.org/13665/diff/1/3#newcode1223 Line 1223: Result temp = frame_->Pop(); Must be a better name. It's not really a temp, it's one of the operands of the comparison. http://codereview.chromium.org/13665/diff/1/3#newcode1225 Line 1225: Register r1 = allocator()->Allocate(); This whole operation (converting a constant result into a register result) should be abstracted as an operation on the result class. http://codereview.chromium.org/13665/diff/1/3#newcode1227 Line 1227: __ mov(r1, Immediate(temp.handle())); // ?? Use __ Set when the rhs is an immediate. http://codereview.chromium.org/13665/diff/1/3#newcode1229 Line 1229: // so r1 is not double counted, so fine? I'm not sure what the comment means, but this should be sorted out. My preference is that we never *explicitly* construct result values outside of the virtual frame and the result class itself (possibly the register allocator). http://codereview.chromium.org/13665/diff/1/3#newcode3989 Line 3989: frame_->SpillAll(); I don't think you need to spill before the call to Load (but it doesn't hurt too much at this intermediate stage, and maybe we should start putting a big comment that says "this is spilled code"...). http://codereview.chromium.org/13665/diff/1/3#newcode4137 Line 4137: frame_->SpillAll(); Again, removing this should be safe (try it!). http://codereview.chromium.org/13665/diff/1/3#newcode4171 Line 4171: // TODO HERE ??? http://codereview.chromium.org/13665/diff/1/2 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/13665/diff/1/2#newcode53 Line 53: // We have called Unuse() before Result goes out of scope. Comment seems out of date. http://codereview.chromium.org/13665 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
