ARM: Add instruction VFPCompareAndSetFlags to macro assembler

ARM: Add instructions VFPCompareAndSetFlags and VFPCompareAndLoadFlags to macro assembler

Also removed the SBit from the vcmp instruction as it is not part of the instruction.

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

[Karl Klose, 2011-01-10 10:43:12]

LGTM.

[Rodolph Perfetta, 2011-01-10 11:24:10]

drive by comments.

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:585: void VFPCompareAndSetFlags(const
DwVfpRegister src1,
Since the instruction sets the flags, the s parameter is meaningless. In fact
the s parameter in the vcmp instruction is also superfluous and probably should
be removed, it is not present in the instruction encoding.

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:590: vmrs(pc);
vmrs should be conditional too.

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:592: void VFPCompareAndSetFlags(const
DwVfpRegister src1,
As above.

[Søren Thygesen Gjesse, 2011-01-10 12:36:01]

New version uploaded.

Rodolph: Please look at the changes in code-stubs-arm.cc - didn't we miss the
clearing of the cumulative exception flags when testing for NaNs before
(condition vs)?

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:585: void VFPCompareAndSetFlags(const
DwVfpRegister src1,
On 2011/01/10 11:24:10, Rodolph Perfetta wrote:
> Since the instruction sets the flags, the s parameter is meaningless. In fact
> the s parameter in the vcmp instruction is also superfluous and probably
should
> be removed, it is not present in the instruction encoding.

Done.

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:590: vmrs(pc);
On 2011/01/10 11:24:10, Rodolph Perfetta wrote:
> vmrs should be conditional too.

Done.

http://codereview.chromium.org/6117003/diff/7001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:592: void VFPCompareAndSetFlags(const
DwVfpRegister src1,
On 2011/01/10 11:24:10, Rodolph Perfetta wrote:
> As above.

Done.

[Rodolph Perfetta, 2011-01-10 14:59:36]

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:977: __ VFPCompareAndSetFlags(d7, d6, r0);
No need to clear the flags, if the result is unordered the overflow flag will be
set (the vs condition).

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1097: __ VFPCompareAndSetFlags(d1, 0.0, scratch);
Ditto.

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4915: __ VFPCompareAndSetFlags(d0, d1, r2);
Ditto.

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:531: void
MacroAssembler::VFPCompareAndSetFlags(const DwVfpRegister src1,
If the flags are cleared the intent is to read them shortly after, so the case
where scratch is used sounds redundant with VFPCompareAndLoad.

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:546: void
MacroAssembler::VFPCompareAndSetFlags(const DwVfpRegister src1,
Ditto.

[Søren Thygesen Gjesse, 2011-01-11 08:56:17]

Rodolph, please explain if we ever need to clear any FPSCR flags when using
vcmpe - from the change introducing the clearing I assumed that they where
"sticky".

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:977: __ VFPCompareAndSetFlags(d7, d6, r0);
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> No need to clear the flags, if the result is unordered the overflow flag will
be
> set (the vs condition).

OK, then I am a bit in the dark regarding the clearing of the IOC bit just
before calling vcmp in http://codereview.chromium.org/6142004/ - could you
please explain when to clear this bit before vcmp and when not to. Having
re-read the ARM ARM I actually don't see any need to clear it when using the
vcmpe instruction (which is what we do).

[Rodolph Perfetta, 2011-01-11 11:39:48]

On 2011/01/11 08:56:17, Søren Gjesse wrote:
> Rodolph, please explain if we ever need to clear any FPSCR flags when using
> vcmpe - from the change introducing the clearing I assumed that they where
> "sticky".

The invalid exception flag is sticky, NZCV are not.

> http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:977: __ VFPCompareAndSetFlags(d7, d6, r0);
> On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> > No need to clear the flags, if the result is unordered the overflow flag
will
> be
> > set (the vs condition).
> 
> OK, then I am a bit in the dark regarding the clearing of the IOC bit just
> before calling vcmp in http://codereview.chromium.org/6142004/ - could you
> please explain when to clear this bit before vcmp and when not to. Having
> re-read the ARM ARM I actually don't see any need to clear it when using the
> vcmpe instruction (which is what we do).

In ARM terminology IOC is a cumulative flag, when set to 1 indicates an invalid
operation occurred since 0 was last written to it. You can find the description
in the ARM ARM page A2-30.

NZCV (from FPSCR) are different and get overwritten at every vcmp.

[Søren Thygesen Gjesse, 2011-01-11 12:41:52]

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:977: __ VFPCompareAndSetFlags(d7, d6, r0);
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> No need to clear the flags, if the result is unordered the overflow flag will
be
> set (the vs condition).

Done.

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1097: __ VFPCompareAndSetFlags(d1, 0.0, scratch);
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> Ditto.

Done.

http://codereview.chromium.org/6117003/diff/6/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4915: __ VFPCompareAndSetFlags(d0, d1, r2);
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> Ditto.

Done.

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:531: void
MacroAssembler::VFPCompareAndSetFlags(const DwVfpRegister src1,
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> If the flags are cleared the intent is to read them shortly after, so the case
> where scratch is used sounds redundant with VFPCompareAndLoad.

Done.

http://codereview.chromium.org/6117003/diff/6/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:546: void
MacroAssembler::VFPCompareAndSetFlags(const DwVfpRegister src1,
On 2011/01/10 14:59:36, Rodolph Perfetta wrote:
> Ditto.

Done.