Allow non-spilled frames into VisitCompareOperation, and allow them...

Allow non-spilled frames into VisitCompareOperation, and allow them
to be passed from there to SmiComparison and SmiComparisonDeferred.
Profit from constants and registers in SmiComparison.

Committed: http://code.google.com/p/v8/source/detail?r=979

[William Hesse, 2008-12-09 15:24:03]

These changes should allow comparison with a SMI constant to leave a variable in
a register.

[Kevin Millikin (Chromium), 2008-12-09 19:00:50]

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.

[iposva, 2008-12-10 01:04:19]

This looks very unfinished, but at least I now see how you intend to use the
Result class.

-Ivan

http://codereview.chromium.org/13665/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/1/3#newcode1183
Line 1183: int value,
Generally I would expect the order of parameters for operation values as left to
right. So you will want to change the order passing the reg before the value.

http://codereview.chromium.org/13665/diff/1/3#newcode1225
Line 1225: Register r1 = allocator()->Allocate();
I do not understand why you bother loading the constant value into a register in
this case. You are comparing two constants so the outcome should be determined
at compile time and you can branch straight to the appropriate label.

P.S. r1 is a very unlucky choice for a register variable name. I know that it
does not matter for the IA32 code gen. In a sense you would have wanted to use
temp_reg here, but temp was already used (and Kevin already commented on that
name).

[William Hesse, 2008-12-10 16:15:25]

OK, some comments addressed.  Other changes, to MergeTo, moved to separate
change list.  No new review needed yet.

http://codereview.chromium.org/13665/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/1/3#newcode1183
Line 1183: int value,
On 2008/12/10 01:04:19, iposva wrote:
> Generally I would expect the order of parameters for operation values as left
to
> right. So you will want to change the order passing the reg before the value.

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1184
Line 1184: Register arg_1)
On 2008/12/09 19:00:50, Kevin Millikin wrote:
> There must be a better name.

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1212
Line 1212: // TODO
On 2008/12/09 19:00:50, Kevin Millikin wrote:
> ???

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1223
Line 1223: Result temp = frame_->Pop();
On 2008/12/09 19:00:50, Kevin Millikin wrote:
> Must be a better name.  It's not really a temp, it's one of the operands of
the
> comparison.

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1225
Line 1225: Register r1 = allocator()->Allocate();
On 2008/12/10 01:04:19, iposva wrote:
> I do not understand why you bother loading the constant value into a register
in
> this case. You are comparing two constants so the outcome should be determined
> at compile time and you can branch straight to the appropriate label.
> 
> P.S. r1 is a very unlucky choice for a register variable name. I know that it
> does not matter for the IA32 code gen. In a sense you would have wanted to use
> temp_reg here, but temp was already used (and Kevin already commented on that
> name).

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1227
Line 1227: __ mov(r1, Immediate(temp.handle()));  // ??
On 2008/12/09 19:00:50, Kevin Millikin wrote:
> Use __ Set when the rhs is an immediate.

Done.

http://codereview.chromium.org/13665/diff/1/3#newcode1229
Line 1229: // so r1 is not double counted, so fine?
On 2008/12/09 19:00:50, Kevin Millikin wrote:
> 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).

Done.

[Kevin Millikin (Chromium), 2008-12-10 16:29:30]

Looking better, but still some comments.  It will be great to see it in place
because it blocks most loop bodies.

I think you lost the automatic Unuse of the register in the destructor for
is_register() and !is_no(reg) results---make sure that gets in in this change or
the next one if its needed.

http://codereview.chromium.org/13665/diff/15/16
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/15/16#newcode1205
Line 1205: VirtualFrame* frame_;
The frame is not needed here---the superclass will handle it.

http://codereview.chromium.org/13665/diff/15/16#newcode1218
Line 1218: // TODO Add in entry and exit frames for deferred code.
This should already be plumbed in through the superclass and the code generator.
 Format todos on the branch as "TODO(): " for now, unless we file an issue.

http://codereview.chromium.org/13665/diff/15/16#newcode1235
Line 1235: Register comparee_reg = allocator()->Allocate();
I still think this whole operation will be common enough to have its own
abstraction, something like Result::ToRegister.

http://codereview.chromium.org/13665/diff/15/16#newcode3998
Line 3998: frame_->SpillAll();
This should be safe to remove---the call to Load will spill if it needs to.

http://codereview.chromium.org/13665/diff/15/16#newcode4146
Line 4146: frame_->SpillAll();
Could be removed.

http://codereview.chromium.org/13665/diff/15/16#newcode4156
Line 4156: frame_->SpillAll();
Ditto.

http://codereview.chromium.org/13665/diff/15/16#newcode4174
Line 4174: frame_->SpillAll();
I think everything will work if we remove the SpillAll here and after the call
to load, since SmiComparison is fixed up.

http://codereview.chromium.org/13665/diff/15/16#newcode4186
Line 4186: frame_->SpillAll();
You could also remove this one.

[Kevin Millikin (Chromium), 2008-12-11 10:24:16]

I patched this code into my workspace, and seems to generate plausible code.  We
will need (as a separate change) to handle the deferred code properly.  A few
more short comments.

http://codereview.chromium.org/13665/diff/15/16
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/15/16#newcode1194
Line 1194: left_side_(left_side), right_side_(right_side) {
The style is to put each initializer on its own line.

http://codereview.chromium.org/13665/diff/15/16#newcode1212
Line 1212: frame_->SpillAll();
The frame will already be spilled on entry.

http://codereview.chromium.org/13665/diff/15/16#newcode1235
Line 1235: Register comparee_reg = allocator()->Allocate();
I would call this register "temp", because it is not (yet) the comparee_reg and
it's confusing to have comparee and comparee_reg at the same time (and have them
be different).

http://codereview.chromium.org/13665/diff/15/16#newcode1237
Line 1237: __ Set(comparee_reg, Immediate(temp.handle()));
What's temp?

http://codereview.chromium.org/13665/diff/15/16#newcode1238
Line 1238: comparee = Result(comparee_reg, this);  // Usage count has net change
0.
Seems wrong.  The allocator has incremented the reference count and the
constructor for the Result has too, so the count is now 2.

We should avoid explicitly calling the constructor for Result in the code
generator itself.

http://codereview.chromium.org/13665/diff/15/16#newcode1248
Line 1248: deferred->exit()->Bind();
At some point, the reference to the result became dead.  Probably right after
the cmp instruction.  We will have to track registers through the deferred code
and should talk about it in person.

[Kevin Millikin (Chromium), 2008-12-11 15:29:55]

You should probably swap the comparee.Unuse() and bind of the deferred exit
label (I'm pretty sure we will need it that way soon).

Otherwise, LGTM with a couple of questions.

http://codereview.chromium.org/13665/diff/215/216
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/215/216#newcode1234
Line 1234: comparee.ConvertConstantToRegister();
Maybe this should be more a more general ToRegister?  We might eventually have
non-register types other than constants, and we might also want an overloaded
version to move a Result (even a register one) to a specific register like in
SmiComparisonDeferred::Generate just above (the Result left_side_ is moved into
edx, which shouldn't require emitting any code if it is already in edx).

http://codereview.chromium.org/13665/diff/215/216#newcode1245
Line 1245: comparee.Unuse();
I think comparee should be unused right before the bind of exit rather than
after.  The register will not be live on either path leading to the label.

http://codereview.chromium.org/13665/diff/215/217
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13665/diff/215/217#newcode56
Line 56: type_ = INVALID;
Is it useful that unusing a constant turns it into invalid?  It might be more
useful if the semantics is unuse if a register, otherwise do nothing.

[iposva, 2008-12-11 19:10:31]

http://codereview.chromium.org/13665/diff/215/216
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/215/216#newcode1233
Line 1233: // TODO Could be optimized to a jump at compile time.
TODOs should have a bug number associated with it. I know this is on a branch
and if you intend to fix it before work on this branch is merged to
bleeding_edge, then it is OK to leave it like this.

http://codereview.chromium.org/13665/diff/215/218
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13665/diff/215/218#newcode75
Line 75: void ConvertConstantToRegister();
How about just calling this function Load() and that function should do the
right thing based on this item's type? Then you could use it as follows:

  Item comparee = frame_->Pop();
  comparee.Load();
  ASSERT(comparee.is_register());  // This ASSERT is unneeded as comparee.reg()
does make sure that reg() is not called on items that are not of register type.
It is left here for demonstration.
  __ test(comparee.reg(), Immediate(kSmiTagMask));


If you don't do it this way you will distribute the checks all over the code
generator and they will keep growing when we eventually add memory based items.
Overall this will lead to toit'er code.

http://codereview.chromium.org/13665/diff/215/218#newcode77
Line 77: enum Type {REGISTER, CONSTANT, INVALID};
In general we list different enum values on separate lines. I know the code
generator is the biggest offender of this rule, but that does not mean we should
propagate that behaviour.

[William Hesse, 2008-12-12 10:17:07]

All comments adressed.  New version uploaded.

http://codereview.chromium.org/13665/diff/15/16
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/15/16#newcode1194
Line 1194: left_side_(left_side), right_side_(right_side) {
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> The style is to put each initializer on its own line.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1205
Line 1205: VirtualFrame* frame_;
On 2008/12/10 16:29:30, Kevin Millikin wrote:
> The frame is not needed here---the superclass will handle it.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1212
Line 1212: frame_->SpillAll();
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> The frame will already be spilled on entry.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1218
Line 1218: // TODO Add in entry and exit frames for deferred code.
On 2008/12/10 16:29:30, Kevin Millikin wrote:
> This should already be plumbed in through the superclass and the code
generator.
>  Format todos on the branch as "TODO(): " for now, unless we file an issue.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1235
Line 1235: Register comparee_reg = allocator()->Allocate();
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> I would call this register "temp", because it is not (yet) the comparee_reg
and
> it's confusing to have comparee and comparee_reg at the same time (and have
them
> be different).

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1235
Line 1235: Register comparee_reg = allocator()->Allocate();
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> I would call this register "temp", because it is not (yet) the comparee_reg
and
> it's confusing to have comparee and comparee_reg at the same time (and have
them
> be different).

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1235
Line 1235: Register comparee_reg = allocator()->Allocate();
On 2008/12/10 16:29:30, Kevin Millikin wrote:
> I still think this whole operation will be common enough to have its own
> abstraction, something like Result::ToRegister.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1237
Line 1237: __ Set(comparee_reg, Immediate(temp.handle()));
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> What's temp?

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1238
Line 1238: comparee = Result(comparee_reg, this);  // Usage count has net change
0.
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> Seems wrong.  The allocator has incremented the reference count and the
> constructor for the Result has too, so the count is now 2.
> 
> We should avoid explicitly calling the constructor for Result in the code
> generator itself.

Done.

http://codereview.chromium.org/13665/diff/15/16#newcode1248
Line 1248: deferred->exit()->Bind();
On 2008/12/11 10:24:16, Kevin Millikin wrote:
> At some point, the reference to the result became dead.  Probably right after
> the cmp instruction.  We will have to track registers through the deferred
code
> and should talk about it in person.

Done.

http://codereview.chromium.org/13665/diff/215/216#newcode1233
Line 1233: if (comparee.is_constant()) {
On 2008/12/11 19:10:31, iposva wrote:
> TODOs should have a bug number associated with it. I know this is on a branch
> and if you intend to fix it before work on this branch is merged to
> bleeding_edge, then it is OK to leave it like this.

Done.

http://codereview.chromium.org/13665/diff/215/216#newcode1234
Line 1234: // TODO Could be optimized to a jump at compile time.
On 2008/12/11 15:29:55, Kevin Millikin wrote:
> Maybe this should be more a more general ToRegister?  We might eventually have
> non-register types other than constants, and we might also want an overloaded
> version to move a Result (even a register one) to a specific register like in
> SmiComparisonDeferred::Generate just above (the Result left_side_ is moved
into
> edx, which shouldn't require emitting any code if it is already in edx).

ToRegister implemented and used.
ToRegister(Register target) not yet implemented, but should be.

http://codereview.chromium.org/13665/diff/215/216#newcode1245
Line 1245: deferred->enter()->Branch(not_zero, not_taken);
On 2008/12/11 15:29:55, Kevin Millikin wrote:
> I think comparee should be unused right before the bind of exit rather than
> after.  The register will not be live on either path leading to the label.

Done.

http://codereview.chromium.org/13665/diff/215/217
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13665/diff/215/217#newcode56
Line 56: type_ = INVALID;
On 2008/12/11 15:29:55, Kevin Millikin wrote:
> Is it useful that unusing a constant turns it into invalid?  It might be more
> useful if the semantics is unuse if a register, otherwise do nothing.

I think it is useful that unuse turns a result invalid in all cases.  If there
is a case where we really want to use a constant after we are done with the
register case, we can reconsider.

http://codereview.chromium.org/13665/diff/215/218
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13665/diff/215/218#newcode75
Line 75: void ConvertConstantToRegister();
On 2008/12/11 19:10:31, iposva wrote:
> How about just calling this function Load() and that function should do the
> right thing based on this item's type? Then you could use it as follows:
> 

Function does as you suggest, but is called ToRegister().
We will also implement ToRegister(Register target) which
should move a result to a specified register, freeing its
previous register if it exists.
>   Item comparee = frame_->Pop();
>   comparee.Load();
>   ASSERT(comparee.is_register());  // This ASSERT is unneeded as
comparee.reg()
> does make sure that reg() is not called on items that are not of register
type.
> It is left here for demonstration.
>   __ test(comparee.reg(), Immediate(kSmiTagMask));
> 
> 
> If you don't do it this way you will distribute the checks all over the code
> generator and they will keep growing when we eventually add memory based
items.
> Overall this will lead to toit'er code.

http://codereview.chromium.org/13665/diff/215/218#newcode77
Line 77: enum Type {REGISTER, CONSTANT, INVALID};
On 2008/12/11 19:10:31, iposva wrote:
> In general we list different enum values on separate lines. I know the code
> generator is the biggest offender of this rule, but that does not mean we
should
> propagate that behaviour.

Done.

[iposva, 2008-12-12 18:23:03]

Two more formatting comments, otherwise LGTM.

http://codereview.chromium.org/13665/diff/403/28
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/403/28#newcode1220
Line 1220: // TODO Verify that Deferred code entry and exit work properly, with
frames.
TODO -> TODO()

http://codereview.chromium.org/13665/diff/403/30
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13665/diff/403/30#newcode81
Line 81: INVALID};
Closing brace on next line like in all other places in the code.

[Kevin Millikin (Chromium), 2008-12-14 12:51:42]

LGTM too, with one small comment.

http://codereview.chromium.org/13665/diff/403/29
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13665/diff/403/29#newcode72
Line 72: return;
You don't really need return from a void method here.  This whole method is
clearer if you add a function Result::is_valid and write:
void Result::ToRegister() {
  ASSERT(is_valid());
  if (is_constant()) {
    Register reg = cgen_->allocator()->Allocate();
    ASSERT(reg.is_valid());
    cgen_->masm()->Set(reg, Immediate(handle()));
    data_.reg_ = reg;
    type_ = REGISTER;
  }
  ASSERT(is_register());
}

[William Hesse, 2008-12-15 13:42:31]

All changes made.  This and 13344 are committed as revision 979.  Sorry they
aren't separate.

http://codereview.chromium.org/13665/diff/403/28
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/13665/diff/403/28#newcode1220
Line 1220: // TODO Verify that Deferred code entry and exit work properly, with
frames.
On 2008/12/12 18:23:03, iposva wrote:
> TODO -> TODO()

Done. Removed - the class checks.

http://codereview.chromium.org/13665/diff/403/29
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/13665/diff/403/29#newcode72
Line 72: return;
On 2008/12/14 12:51:42, Kevin Millikin wrote:
> You don't really need return from a void method here.  This whole method is
> clearer if you add a function Result::is_valid and write:
> void Result::ToRegister() {
>   ASSERT(is_valid());
>   if (is_constant()) {
>     Register reg = cgen_->allocator()->Allocate();
>     ASSERT(reg.is_valid());
>     cgen_->masm()->Set(reg, Immediate(handle()));
>     data_.reg_ = reg;
>     type_ = REGISTER;
>   }
>   ASSERT(is_register());
> }

Done.

http://codereview.chromium.org/13665/diff/403/30
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/13665/diff/403/30#newcode81
Line 81: INVALID};
On 2008/12/12 18:23:03, iposva wrote:
> Closing brace on next line like in all other places in the code.

Done.