Re: Relax requirement from VFP3 to VFP2 where possible. (issue 10818026)

[yan...@chromium.org]

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#newcode50
> > 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#newcode513
> > 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#newcode539
> > 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#newcode790
> > 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.

http://codereview.chromium.org/10818026/

[rodolph....@gmail.com]

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#newcode132
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#newcode1924
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#newcode1981
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#newcode3990
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#newcode2096
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#newcode136
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 "

http://codereview.chromium.org/10818026/

[yan...@chromium.org]

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#newcode132
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#newcode1924
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#newcode1981
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#newcode2096
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#newcode136
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.

http://codereview.chromium.org/10818026/

[rodolph....@gmail.com]

> 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#newcode132
> 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#newcode136
> 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.

http://codereview.chromium.org/10818026/

[rodolph....@gmail.com]

LGTM.

http://codereview.chromium.org/10818026/