Relax requirement from VFP3 to VFP2 where possible.

Relax requirement from VFP3 to VFP2 where possible.

BUG=
TEST=

Committed: https://code.google.com/p/v8/source/detail?r=12194

[Yang, 2012-07-24 13:52:36]

This change is mostly mechanical. I'm mostly concerned with the change I did to
Vmov in macro-assembler-arm.cc and the way I try to detect VFP2 by matching to
"vfp" in /proc/cpuinfo

Also, it would be great if we could test this patch on a few pieces different
real ARM hardware :)

Please take a look.

[Sven Panne, 2012-07-25 07:49:06]

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

http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.cc#newco...
src/arm/assembler-arm.cc:50: unsigned CpuFeatures::supported_ = 0;
We should either use uint64_t here or use unsigned below (same for other
uint64_t/unsigned mixing below).

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

http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
src/arm/assembler-arm.h:513: if (f == VFP2 && !FLAG_enable_vfp3) return false;
This is confusing, I think we should add a new flag for VFP2 or rename the
existing one. In any case our build bots have to be changed accordingly.

http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
src/arm/assembler-arm.h:539: // VFP2 and ARMv2 are implied by VFP3.
ARMv7?

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

http://codereview.chromium.org/10818026/diff/1/src/arm/macro-assembler-arm.cc...
src/arm/macro-assembler-arm.cc:790: } else if (false &&
CpuFeatures::IsSupported(VFP3)) {
As discussed offline, this duplicates some fallback code in Assembler:vmov, so
we should handle VFP2-only platforms there.

[Yang, 2012-07-25 09:48:49]

On 2012/07/25 07:49:06, Sven Panne wrote:
> http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (right):
> 
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.cc#newco...
> src/arm/assembler-arm.cc:50: unsigned CpuFeatures::supported_ = 0;
> We should either use uint64_t here or use unsigned below (same for other
> uint64_t/unsigned mixing below).
> 
> http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h
> File src/arm/assembler-arm.h (right):
> 
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
> src/arm/assembler-arm.h:513: if (f == VFP2 && !FLAG_enable_vfp3) return false;
> This is confusing, I think we should add a new flag for VFP2 or rename the
> existing one. In any case our build bots have to be changed accordingly.
> 
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
> src/arm/assembler-arm.h:539: // VFP2 and ARMv2 are implied by VFP3.
> ARMv7?
> 
> http://codereview.chromium.org/10818026/diff/1/src/arm/macro-assembler-arm.cc
> File src/arm/macro-assembler-arm.cc (right):
> 
>
http://codereview.chromium.org/10818026/diff/1/src/arm/macro-assembler-arm.cc...
> src/arm/macro-assembler-arm.cc:790: } else if (false &&
> CpuFeatures::IsSupported(VFP3)) {
> As discussed offline, this duplicates some fallback code in Assembler:vmov, so
> we should handle VFP2-only platforms there.

I addressed Sven's comments. Following Rodolph's comments (crankshaft has been
consciously ruled out for processors without VFP3), I decided to scale back this
CL to only lower the requirements to VFP2 where possible.

[Yang, 2012-07-25 09:49:24]

On 2012/07/25 09:48:49, Yang wrote:
> On 2012/07/25 07:49:06, Sven Panne wrote:
> > http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.cc
> > File src/arm/assembler-arm.cc (right):
> > 
> >
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.cc#newco...
> > src/arm/assembler-arm.cc:50: unsigned CpuFeatures::supported_ = 0;
> > We should either use uint64_t here or use unsigned below (same for other
> > uint64_t/unsigned mixing below).
> > 
> > http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h
> > File src/arm/assembler-arm.h (right):
> > 
> >
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
> > src/arm/assembler-arm.h:513: if (f == VFP2 && !FLAG_enable_vfp3) return
false;
> > This is confusing, I think we should add a new flag for VFP2 or rename the
> > existing one. In any case our build bots have to be changed accordingly.
> > 
> >
>
http://codereview.chromium.org/10818026/diff/1/src/arm/assembler-arm.h#newcod...
> > src/arm/assembler-arm.h:539: // VFP2 and ARMv2 are implied by VFP3.
> > ARMv7?
> > 
> >
http://codereview.chromium.org/10818026/diff/1/src/arm/macro-assembler-arm.cc
> > File src/arm/macro-assembler-arm.cc (right):
> > 
> >
>
http://codereview.chromium.org/10818026/diff/1/src/arm/macro-assembler-arm.cc...
> > src/arm/macro-assembler-arm.cc:790: } else if (false &&
> > CpuFeatures::IsSupported(VFP3)) {
> > As discussed offline, this duplicates some fallback code in Assembler:vmov,
so
> > we should handle VFP2-only platforms there.
> 
> I addressed Sven's comments. Following Rodolph's comments (crankshaft has been
> consciously ruled out for processors without VFP3), I decided to scale back
this
> CL to only lower the requirements to VFP2 where possible.

Please take another quick look.

[Rodolph Perfetta, 2012-07-25 10:59:54]

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

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:132: ASSERT(!IsSupported(VFP3) || (IsSupported(VFP2) &&
IsSupported(ARMv7)));
It is possible to have an ARMv7 core without VFP and in this case the assert
would fail. It is an uncommon configuration though.

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1924: ASSERT(CpuFeatures::IsEnabled(VFP2));
I would leave VFP3 as a requirement as this function only makes sense for VFPv3.

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1981: CpuFeatures::IsSupported(VFP3)) {
Reverse the order of the test, then FitsVMOVDoubleImmediate can assert VFP3
support.

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

http://codereview.chromium.org/10818026/diff/3002/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:3990: __ Vmov(kDoubleRegZero, 0.0);
The Vmov macro will use kDoubleRegZero if one wont to move 0.0 into a VFP reg.
The kdoubleRegZero is initialised with the value 0.0 here so you need to use the
instruction vmov not the marcro instruction.

http://codereview.chromium.org/10818026/diff/3002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/10818026/diff/3002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:2096: CpuFeatures::Scope scope_vfp3(VFP2);
scope_vfp2

http://codereview.chromium.org/10818026/diff/3002/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10818026/diff/3002/src/platform-linux.cc#newco...
src/platform-linux.cc:136: search_string = "vfp";
this will match the vfpv3 string since when they differ the search_string will
have reach '\0'.

Maybe put "vfp "

[Yang, 2012-07-25 12:09:32]

Uploaded a new patch set based on the comments. Some of those comments confuse
me, I replied to those.

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

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:132: ASSERT(!IsSupported(VFP3) || (IsSupported(VFP2) &&
IsSupported(ARMv7)));
On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> It is possible to have an ARMv7 core without VFP and in this case the assert
> would fail. It is an uncommon configuration though.

This only tests that VFP3 -> (VFP2 && ARMv7)
if ARMv7 is supported and VFP3 is not, then the assert still passes.

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1924: ASSERT(CpuFeatures::IsEnabled(VFP2));
On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> I would leave VFP3 as a requirement as this function only makes sense for
VFPv3.

Done.

http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1981: CpuFeatures::IsSupported(VFP3)) {
On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> Reverse the order of the test, then FitsVMOVDoubleImmediate can assert VFP3
> support.

Done.

http://codereview.chromium.org/10818026/diff/3002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/10818026/diff/3002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:2096: CpuFeatures::Scope scope_vfp3(VFP2);
On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> scope_vfp2

Done.

http://codereview.chromium.org/10818026/diff/3002/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10818026/diff/3002/src/platform-linux.cc#newco...
src/platform-linux.cc:136: search_string = "vfp";
On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> this will match the vfpv3 string since when they differ the search_string will
> have reach '\0'.
> 
> Maybe put "vfp "

since VFP3 implies VFP2, wouldn't this be fine? I'm concerned that if the
cpuinfo string happens to end with "vfp", "vfp " wouldn't match.

[Rodolph Perfetta, 2012-07-25 12:44:09]

> http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (right):
> 
>
http://codereview.chromium.org/10818026/diff/3002/src/arm/assembler-arm.cc#ne...
> src/arm/assembler-arm.cc:132: ASSERT(!IsSupported(VFP3) || (IsSupported(VFP2)
&&
> IsSupported(ARMv7)));
> On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> > It is possible to have an ARMv7 core without VFP and in this case the assert
> > would fail. It is an uncommon configuration though.
> 
> This only tests that VFP3 -> (VFP2 && ARMv7)
> if ARMv7 is supported and VFP3 is not, then the assert still passes.

You are right, my mistake.

>
http://codereview.chromium.org/10818026/diff/3002/src/platform-linux.cc#newco...
> src/platform-linux.cc:136: search_string = "vfp";
> On 2012/07/25 10:59:54, Rodolph Perfetta wrote:
> > this will match the vfpv3 string since when they differ the search_string
will
> > have reach '\0'.
> > 
> > Maybe put "vfp "
> 
> since VFP3 implies VFP2, wouldn't this be fine? I'm concerned that if the
> cpuinfo string happens to end with "vfp", "vfp " wouldn't match.

Yes you are right this would be fine.

[Rodolph Perfetta, 2012-07-25 12:44:39]

LGTM.

[Adam Malcontenti-Wilson, 2012-08-13 13:57:52]

Just was looking over the common.gypi file, and I thought that
CAN_USE_VFP_INSTRUCTIONS is no longer used in favour of the VFP2 and VFP3
variants. 
Changing to CAN_USE_VFP2_INSTRUCTIONS would save having to do the /proc/cpuinfo
check on hard float systems where I thought the assumption was that it must
support at least VFP2.

http://codereview.chromium.org/10818026/diff/15001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10818026/diff/15001/build/common.gypi#newcode149
build/common.gypi:149: 'CAN_USE_VFP_INSTRUCTIONS',
Should this be CAN_USE_VFP2_INSTRUCTIONS instead?