Inline floating point compare...

Inline floating point compare

Inline floating point compare instead of calling the stub when the following conditions are met:
  * Code is in a loop
  * Compare is not a for loop condition
  * Compare is not an equal comparison

This inlined code handles heap number to heap number and heap number to smi compare. It can also handle smi to smi compare, but whenever there is a chance of comparing two smis the smi compare is inlined before the inlined floating point compare. Support for non SSE2 hardware is included.

A new set of variants of the compare stub without the floating point comparison code is called if the inline comapre fails due to the operands not beeing heap numbers or smis.

The virtual frame has been extended with a branch taking two live results to be carried through to the destination. This makes this change much simpler as the inlined code have two live results in registers and a number of bailouts.

CompareStub::GetName needs to be updated as well. I will do that as a separate change.

Also inlined equality check if both operands can't be NaN. This can only provide positive equals if it is the same object.

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

[Mads Ager (chromium), 2010-03-23 09:41:55]

LGTM with a couple of comments.

http://codereview.chromium.org/1117011/diff/1/5
File src/codegen.h (right):

http://codereview.chromium.org/1117011/diff/1/5#newcode369
src/codegen.h:369: // the stub will be called due to number comparison not
working.
will be called if one of the operands is not a number?

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

http://codereview.chromium.org/1117011/diff/1/2#newcode2367
src/ia32/codegen-ia32.cc:2367: // Convert fron signed to unsigned comparison to
match the way EFLAGS are set
fron -> from

http://codereview.chromium.org/1117011/diff/1/2#newcode2714
src/ia32/codegen-ia32.cc:2714: // Condition for inlining the number compare.
Maybe describe the condition in words here (as in the changelist description)
and describe the reason for this condition.

http://codereview.chromium.org/1117011/diff/1/2#newcode2769
src/ia32/codegen-ia32.cc:2769: // objects are the same they are equal, if not
continue to compare stub.
Remove the ", if not continue to compare stub." since you can hit the inline
number comparison?

http://codereview.chromium.org/1117011/diff/1/2#newcode2805
src/ia32/codegen-ia32.cc:2805: // Check that the comparison operand is a number
.Jump to not_numbers jump
" .Jump" -> ". Jump"

http://codereview.chromium.org/1117011/diff/1/2#newcode2806
src/ia32/codegen-ia32.cc:2806: // target passig the left and right result if the
operand is not a number.
passig -> passing

http://codereview.chromium.org/1117011/diff/1/2#newcode2862
src/ia32/codegen-ia32.cc:2862: // Load a comparison operand onto into a XMM
register. Jump to not_numbers jump
onto into -> into

http://codereview.chromium.org/1117011/diff/1/2#newcode2863
src/ia32/codegen-ia32.cc:2863: // target passig the left and right result if the
operand is not a number.
passig -> passing

http://codereview.chromium.org/1117011/diff/1/2#newcode2891
src/ia32/codegen-ia32.cc:2891: FieldOperand(operand->reg(),
HeapNumber::kValueOffset));
Seems this would fit on one line?

http://codereview.chromium.org/1117011/diff/1/2#newcode2926
src/ia32/codegen-ia32.cc:2926: CheckComparisonOperand(masm_, left_side,
left_side, right_side,
It is not clear to me what we gain by having CheckComparisonOperand separately
from LoadComparisonOperand?  In the case where we have no static info, don't we
just end up repeating smi checks?

http://codereview.chromium.org/1117011/diff/1/2#newcode2937
src/ia32/codegen-ia32.cc:2937: // Bail out of a NaN is involved.
of -> if

http://codereview.chromium.org/1117011/diff/1/2#newcode11021
src/ia32/codegen-ia32.cc:11021: Label number_comparison_failed;
non_number_comparison?

http://codereview.chromium.org/1117011/diff/1/2#newcode11030
src/ia32/codegen-ia32.cc:11030: // Don't base result in EFLAGS when a NaN is
involved.
in -> on?

[Søren Thygesen Gjesse, 2010-03-23 10:19:08]

http://codereview.chromium.org/1117011/diff/1/5
File src/codegen.h (right):

http://codereview.chromium.org/1117011/diff/1/5#newcode369
src/codegen.h:369: // the stub will be called due to number comparison not
working.
On 2010/03/23 09:41:55, Mads Ager wrote:
> will be called if one of the operands is not a number?

Better comment, changed.

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

http://codereview.chromium.org/1117011/diff/1/2#newcode2367
src/ia32/codegen-ia32.cc:2367: // Convert fron signed to unsigned comparison to
match the way EFLAGS are set
On 2010/03/23 09:41:55, Mads Ager wrote:
> fron -> from

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2714
src/ia32/codegen-ia32.cc:2714: // Condition for inlining the number compare.
On 2010/03/23 09:41:55, Mads Ager wrote:
> Maybe describe the condition in words here (as in the changelist description)
> and describe the reason for this condition.

Done. Added reasons for excluding equal and for-loop conditions (the in loop
should be obvious).

http://codereview.chromium.org/1117011/diff/1/2#newcode2769
src/ia32/codegen-ia32.cc:2769: // objects are the same they are equal, if not
continue to compare stub.
On 2010/03/23 09:41:55, Mads Ager wrote:
> Remove the ", if not continue to compare stub." since you can hit the inline
> number comparison?

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2805
src/ia32/codegen-ia32.cc:2805: // Check that the comparison operand is a number
.Jump to not_numbers jump
On 2010/03/23 09:41:55, Mads Ager wrote:
> " .Jump" -> ". Jump"

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2806
src/ia32/codegen-ia32.cc:2806: // target passig the left and right result if the
operand is not a number.
On 2010/03/23 09:41:55, Mads Ager wrote:
> passig -> passing

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2862
src/ia32/codegen-ia32.cc:2862: // Load a comparison operand onto into a XMM
register. Jump to not_numbers jump
On 2010/03/23 09:41:55, Mads Ager wrote:
> onto into -> into

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2863
src/ia32/codegen-ia32.cc:2863: // target passig the left and right result if the
operand is not a number.
On 2010/03/23 09:41:55, Mads Ager wrote:
> passig -> passing

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2891
src/ia32/codegen-ia32.cc:2891: FieldOperand(operand->reg(),
HeapNumber::kValueOffset));
On 2010/03/23 09:41:55, Mads Ager wrote:
> Seems this would fit on one line?

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode2926
src/ia32/codegen-ia32.cc:2926: CheckComparisonOperand(masm_, left_side,
left_side, right_side,
On 2010/03/23 09:41:55, Mads Ager wrote:
> It is not clear to me what we gain by having CheckComparisonOperand separately
> from LoadComparisonOperand?  In the case where we have no static info, don't
we
> just end up repeating smi checks?

This was the cleanest way of ensuring that we always load either zero or two
items on the FPU stack. This is the same pattern as is used by
FloatingPointHelper::CheckFloatOperands/FloatingPointHelper::LoadFloatOperands.
I will look into combining these in a separate change.

http://codereview.chromium.org/1117011/diff/1/2#newcode2937
src/ia32/codegen-ia32.cc:2937: // Bail out of a NaN is involved.
On 2010/03/23 09:41:55, Mads Ager wrote:
> of -> if

Done.

http://codereview.chromium.org/1117011/diff/1/2#newcode11021
src/ia32/codegen-ia32.cc:11021: Label number_comparison_failed;
On 2010/03/23 09:41:55, Mads Ager wrote:
> non_number_comparison?

Better name, done.

http://codereview.chromium.org/1117011/diff/1/2#newcode11030
src/ia32/codegen-ia32.cc:11030: // Don't base result in EFLAGS when a NaN is
involved.
On 2010/03/23 09:41:55, Mads Ager wrote:
> in -> on?

Done.

[Erik Corry, 2010-03-23 18:09:24]

http://codereview.chromium.org/1117011/diff/1009/6002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1117011/diff/1009/6002#newcode2736
src/ia32/codegen-ia32.cc:2736: if (nan_info == kCantBothBeNaN && cc == equal) {
This seems wrong.  If one side is a constant fp number then they can't both be
NaN, but you still have to check whether the other side is a number or a JS
object with an arbitrary ToNumber method with side effects.  If we insisted on
strict equality it might be OK.

[Søren Thygesen Gjesse, 2010-03-23 20:40:01]

http://codereview.chromium.org/1117011/diff/1009/6002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1117011/diff/1009/6002#newcode2736
src/ia32/codegen-ia32.cc:2736: if (nan_info == kCantBothBeNaN && cc == equal) {
On 2010/03/23 18:09:24, Erik Corry wrote:
> This seems wrong.  If one side is a constant fp number then they can't both be
> NaN, but you still have to check whether the other side is a number or a JS
> object with an arbitrary ToNumber method with side effects.  If we insisted on
> strict equality it might be OK.

This will only provide true when the objects are the same, but not false if they
are different. If they are not the same objects all the rest of the compare code
will perform all the rest of the checks.