ARM: Fix comparison of NaN values....

ARM: Fix comparison of NaN values.

Enables the cumulative exception flag when comparing values, and uses it to detect NaN results.

BUG=1023
TEST=none

Landed in revision 6236.

[Mads Ager (chromium), 2011-01-07 11:28:38]

Have you gone through the other uses of vcmp in the current system to verify
that the existing code does not break because of this change? If you have, this
looks good to me. I'll land once you have looked through the existing uses.

Thanks for the patch!

http://codereview.chromium.org/6142004/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6142004/diff/1/src/arm/assembler-arm.cc#newcod...
src/arm/assembler-arm.cc:2338: void Assembler::vcmp(const DwVfpRegister src1,
Should we have a vcmp and a vcmpe instruction? I guess we will always want the
vcmpe version, so probably not?

[Søren Thygesen Gjesse, 2011-01-07 11:44:32]

Drive by comment.

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1082: // Clear the Invalid cumulative exception
flags.
I will suggest a macro assembler instruction for this:

__ ClearFPCSRBit(kVFPInvalidExceptionBit, scratch);

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1126: // Clear the Invalid cumulative exception
flags.
Ditto.

[Alexandre, 2011-01-07 18:34:49]

Thanks for the review.
The patch should be uploaded soon.

Alexandre

http://codereview.chromium.org/6142004/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6142004/diff/1/src/arm/assembler-arm.cc#newcod...
src/arm/assembler-arm.cc:2338: void Assembler::vcmp(const DwVfpRegister src1,
I think we shouldn't.
Given the current use of vcmp, there is no reason we would need the flags not to
be set. Not adding a new variant will also avoid confusion.

Also, I checked the use of all vcmp instructions in the arm code, and having
this E bit looks fine.

On 2011/01/07 11:28:38, Mads Ager wrote:
> Should we have a vcmp and a vcmpe instruction? I guess we will always want the
> vcmpe version, so probably not?

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1082: // Clear the Invalid cumulative exception
flags.
Added the ClearFPSCRBits Masm instruction.
On 2011/01/07 11:44:32, Søren Gjesse wrote:
> I will suggest a macro assembler instruction for this:
> 
> __ ClearFPCSRBit(kVFPInvalidExceptionBit, scratch);

http://codereview.chromium.org/6142004/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1126: // Clear the Invalid cumulative exception
flags.
Used the new pseudo instruction.
On 2011/01/07 11:44:32, Søren Gjesse wrote:
> Ditto.

[Mads Ager (chromium), 2011-01-10 08:05:22]

LGTM, landed in bleeding_edge revision 6236.