Move SpilledFrame inside cases of SmiOperation....

Move SpilledFrame inside cases of SmiOperation.
Change subtraction case of SmiOperation to use unspilled frame.


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

[Kevin Millikin (Chromium), 2009-01-23 08:49:24]

It's early in the day so I could be wrong, but I think we can generate somewhat
better code in the reversed subtract case.

http://codereview.chromium.org/18495/diff/203/204
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18495/diff/203/204#newcode858
Line 858: DeferredInlinedSmiAdd(CodeGenerator* generator, Smi* value,
While you're changing this code, you could clean it up to have one parameter per
line.

http://codereview.chromium.org/18495/diff/203/204#newcode864
Line 864: virtual void Generate() {
I also don't care for these inline-looking Generate functions in the deferred
code classes.  They can't be inlined at the call site anyway.  Feel free to move
it out and right after the class declaration while you're cleaning up this code.

http://codereview.chromium.org/18495/diff/203/204#newcode932
Line 932: // The result is actually returned in eax.
Remove this comment.

http://codereview.chromium.org/18495/diff/203/204#newcode956
Line 956: enter()->Bind(&argument);  // argument contains lhs - rhs.  lhs is
value.
Try to make comments look like English sentences. You should try to capitalize
the first word.  "The argument contains the optimistic subtraction value - rhs."

http://codereview.chromium.org/18495/diff/203/204#newcode958
Line 958: generator()->frame()->Spill(argument.reg());
I'm confused by this spill.  Since argument is not written to, does it need to
be spilled? (Since it's not used as an "lvalue", does it even need to be a
register?)

http://codereview.chromium.org/18495/diff/203/204#newcode963
Line 963: __ sub(temp.reg(), Operand(argument.reg())); // lhs - (lhs - rhs)
gives us rhs.
Line seems too long.  There should be two spaces between the semicolon and the
comment.

http://codereview.chromium.org/18495/diff/203/204#newcode1001
Line 1001: if (!reversed) {
Since this is a two-armed if anyway, it might read better if the condition were
not negated.

http://codereview.chromium.org/18495/diff/203/204#newcode1022
Line 1022: Result answer(this);  // Only allocated a new register if reversed.
allocated ==> allocate

http://codereview.chromium.org/18495/diff/203/204#newcode1024
Line 1024: frame_->Spill(operand.reg());
It looks like operand is only clobbered in the not-reversed case.  Could this
spill be done more lazily?

http://codereview.chromium.org/18495/diff/203/204#newcode1041
Line 1041: deferred->enter()->Branch(overflow, &answer, not_taken);
Maybe it makes this code uglier, but can't we pass operand instead of answer? 
In the non-reversed case, it's the same thing and in the deferred case it avoids
allocating a register, moving a constant into it, and subtracting, which seems
to be just recovering operand.

http://codereview.chromium.org/18495/diff/203/204#newcode1160
Line 1160: if (!reversed) {
Reverse the arms of the if?

[Kevin Millikin (Chromium), 2009-01-23 11:51:28]

LGTM.  You can address my other clean-up comments as a separate changelist if
you want.

[William Hesse, 2012-05-07 09:35:10]

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode858
src/codegen-ia32.cc:858: DeferredInlinedSmiAdd(CodeGenerator* generator, Smi*
value,
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> While you're changing this code, you could clean it up to have one parameter
per
> line.

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode864
src/codegen-ia32.cc:864: virtual void Generate() {
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> I also don't care for these inline-looking Generate functions in the deferred
> code classes.  They can't be inlined at the call site anyway.  Feel free to
move
> it out and right after the class declaration while you're cleaning up this
code.

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode932
src/codegen-ia32.cc:932: // The result is actually returned in eax.
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> Remove this comment.

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode1001
src/codegen-ia32.cc:1001: if (!reversed) {
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> Since this is a two-armed if anyway, it might read better if the condition
were
> not negated.

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode1022
src/codegen-ia32.cc:1022: Result answer(this);  // Only allocated a new register
if reversed.
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> allocated ==> allocate

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode1024
src/codegen-ia32.cc:1024: frame_->Spill(operand.reg());
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> It looks like operand is only clobbered in the not-reversed case.  Could this
> spill be done more lazily?

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode1041
src/codegen-ia32.cc:1041: deferred->enter()->Branch(overflow, &answer,
not_taken);
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> Maybe it makes this code uglier, but can't we pass operand instead of answer? 
> In the non-reversed case, it's the same thing and in the deferred case it
avoids
> allocating a register, moving a constant into it, and subtracting, which seems
> to be just recovering operand.

Done.

http://codereview.chromium.org/18495/diff/203/src/codegen-ia32.cc#newcode1160
src/codegen-ia32.cc:1160: if (!reversed) {
On 2009/01/23 08:49:25, Kevin Millikin wrote:
> Reverse the arms of the if?

Done.