Use registers to pass arguments to GenericBinaryOpStub....

Use registers to pass arguments to GenericBinaryOpStub.
Currently arguments are never passed on registers (due to the way ArgsInRegistersSupported is written) and
if they were, the stub would break in several places because registers are not preserved properly in the
course of execution. This CL makes use of registers more often (than never) and makes sure that registers are
handler properly.
A peformance gain is small (0.2-0.3%) but stable.
This CL was extracted from the one sent out earlier (http://codereview.chromium.org/551093).

[Mads Ager (chromium), 2010-01-25 15:44:03]

LGTM

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

http://codereview.chromium.org/554062/diff/1/2#newcode7506
src/ia32/codegen-ia32.cc:7506: HasArgumentsReversed() ?
Indent by one more space. Looks like 3-space indent instead of 4-space indent?

http://codereview.chromium.org/554062/diff/1/2#newcode7520
src/ia32/codegen-ia32.cc:7520: HasArgumentsReversed() ?
Indent by one more space.

http://codereview.chromium.org/554062/diff/1/2#newcode7571
src/ia32/codegen-ia32.cc:7571: if (mode == OVERWRITE_RIGHT)
Please uses '{' and '}' when having multi-line if statements.

[Vladislav Kaznacheev, 2010-01-25 16:24:47]

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

http://codereview.chromium.org/554062/diff/1/2#newcode7506
src/ia32/codegen-ia32.cc:7506: HasArgumentsReversed() ?
On 2010/01/25 15:44:03, Mads Ager wrote:
> Indent by one more space. Looks like 3-space indent instead of 4-space indent?

Done.

http://codereview.chromium.org/554062/diff/1/2#newcode7520
src/ia32/codegen-ia32.cc:7520: HasArgumentsReversed() ?
On 2010/01/25 15:44:03, Mads Ager wrote:
> Indent by one more space.

Done.

http://codereview.chromium.org/554062/diff/1/2#newcode7571
src/ia32/codegen-ia32.cc:7571: if (mode == OVERWRITE_RIGHT)
On 2010/01/25 15:44:03, Mads Ager wrote:
> Please uses '{' and '}' when having multi-line if statements.

Done.

[Kevin Millikin (Chromium), 2010-01-25 16:27:44]

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

http://codereview.chromium.org/554062/diff/1/2#newcode744
src/ia32/codegen-ia32.cc:744: enum ArgLocation {
It seems like this should be a member of FloatingPointHelper.

http://codereview.chromium.org/554062/diff/1/2#newcode7288
src/ia32/codegen-ia32.cc:7288: if (HasArgumentsInRegisters()) {
Is this right?  What if op_ == Token::ADD or Token::SUB and
!HasArgumentsInRegisters().  Won't the heap allocation target
&after_alloc_failure on a failure, but the label will never be bound?

http://codereview.chromium.org/554062/diff/1/2#newcode7294
src/ia32/codegen-ia32.cc:7294: }
Missing break would be a problem (at least dead code generation) if code was
added to the fall through or clauses were reordered.

Add break or add a "Fall through" comment.  (Explicit break is better).

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

http://codereview.chromium.org/554062/diff/1/3#newcode707
src/ia32/codegen-ia32.h:707: return ((op_ == Token::ADD) || (op_ == Token::SUB))
||
You lost a two-space indent here.

This is a complicated comparison, I like the following better:

bool ArgsInRegistersSupported() {
  if (op_ == Token::ADD || op_ == Token::SUB) return true;
  if (op_ == Token::MUL || op_ == Token::DIV) {
    return NO_SMI_CODE_IN_STUB;
  }
  return false;
}

You might also rename this to SupportsArgsInRegisters, to parallel
HasArgsInRegisters.

http://codereview.chromium.org/554062/diff/1/3#newcode718
src/ia32/codegen-ia32.h:718: bool HasArgumentsInRegisters() { return
args_in_registers_; }
I'm not sure why HasArguments is spelled out but SetArgs is not.  It should be
consistent and I don't mind abbreviating Args.  You could change it since you're
already modifying this code.

[Vladislav Kaznacheev, 2010-01-25 17:44:53]

Sorry, I have committed the code before I saw Kevin's comments. I already fixed
the bug with op_ == Token::ADD or Token::SUB and !HasArgumentsInRegisters(). I
am taking care of the rest of the comments in a separate patch
(http://codereview.chromium.org/543193).

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

http://codereview.chromium.org/554062/diff/1/2#newcode744
src/ia32/codegen-ia32.cc:744: enum ArgLocation {
On 2010/01/25 16:27:44, Kevin Millikin wrote:
> It seems like this should be a member of FloatingPointHelper.

Done.

http://codereview.chromium.org/554062/diff/1/2#newcode7288
src/ia32/codegen-ia32.cc:7288: if (HasArgumentsInRegisters()) {
This was a bug. Fixed.
On 2010/01/25 16:27:44, Kevin Millikin wrote:
> Is this right?  What if op_ == Token::ADD or Token::SUB and
> !HasArgumentsInRegisters().  Won't the heap allocation target
> &after_alloc_failure on a failure, but the label will never be bound?

http://codereview.chromium.org/554062/diff/1/2#newcode7294
src/ia32/codegen-ia32.cc:7294: }
On 2010/01/25 16:27:44, Kevin Millikin wrote:
> Missing break would be a problem (at least dead code generation) if code was
> added to the fall through or clauses were reordered.
> 
> Add break or add a "Fall through" comment.  (Explicit break is better).

Done.

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

http://codereview.chromium.org/554062/diff/1/3#newcode707
src/ia32/codegen-ia32.h:707: return ((op_ == Token::ADD) || (op_ == Token::SUB))
||
On 2010/01/25 16:27:44, Kevin Millikin wrote:
> You lost a two-space indent here.
> 
> This is a complicated comparison, I like the following better:
> 
> bool ArgsInRegistersSupported() {
>   if (op_ == Token::ADD || op_ == Token::SUB) return true;
>   if (op_ == Token::MUL || op_ == Token::DIV) {
>     return NO_SMI_CODE_IN_STUB;
>   }
>   return false;
> }
> 
> You might also rename this to SupportsArgsInRegisters, to parallel
> HasArgsInRegisters.

Done.

http://codereview.chromium.org/554062/diff/1/3#newcode718
src/ia32/codegen-ia32.h:718: bool HasArgumentsInRegisters() { return
args_in_registers_; }
On 2010/01/25 16:27:44, Kevin Millikin wrote:
> I'm not sure why HasArguments is spelled out but SetArgs is not.  It should be
> consistent and I don't mind abbreviating Args.  You could change it since
you're
> already modifying this code.

Done.