rm: First step towards dynamic NEON support.

arm: First step towards dynamic NEON support.

This patch adds minimal support for dynamic ARM NEON support,
i.e. the ability to probe the CPU at runtime for NEON and
provide alternate code paths when it is available.

- Add include/core/SkUtilsArm.h, which declares a few helper
  macros (e.g. SK_NEON_ARM_IS_DYNAMIC), plus the handy
  function 'sk_cpu_arm_has_neon()' which returns true if
  the target CPU supports the ARM NEON instruction set.

  Note that the header is in include/core/ because it will
  have to be included from NEON-specific code under src/code/

  It would probably be more logical to put it under include/opts/
  instead, but this would require moving all the NEON-specific
  stuff under src/code/ into src/opts/, which is not trivial
  due to the way the code is currently architected.

- Add src/core/SkUtilsArm.cpp which implements
  'sk_cpu_arm_has_neon' for ARM-based Linux systems, only
  when SK_NEON_ARM_IS_DYNAMIC is true.

  (For other cases, 'sk_cpu_arm_has_neon' is an inline function
   that returns a constant 'true' or 'false' value).

  There is no user-level accessible CPUID instruction on ARM,
  so do all CPU feature probing by parsing /proc/cpuinfo.
  This is Linux-specific.

  For Debug build types, the CPU probing result is printed
  to the Android log (or Linux command-line) for easier
  debugging.

- Create a new 'opts_neon' target (static library) which shall
  contain all the NEON-specific code paths for the library.

  This is necessary because -mfpu=neon impacts also non-scalar
  code. Just like with -mssse3 on x86, we can't build the rest
  of the library with this flag.

  Note that for now, we only include memset16_neon and
  memset32_neon in this library.

- Modify opts_check_arm.cpp to implement SK_ARM_NEON_IS_DYNAMIC
  properly.

Compared to a 'xoom' build, the only difference is the use of
NEON-optimized memset16/32 functions. Later patches will move
more NEON-specific code paths to 'opts_neon'.
Committed: https://code.google.com/p/skia/source/detail?r=4069

[DerekS, 2012-05-29 19:40:10]

do you want us to start reviewing this or is this CL still a work in progress?

[digit, 2012-05-29 19:44:25]

On 2012/05/29 19:40:10, DerekS wrote:
> do you want us to start reviewing this or is this CL still a work in progress?

the CL should be good by itself. However it looks it's missing a few files
(SkUtilsArm.h, SkUtilsArm.cpp). Let me check this and update a corrected one.

My plan is to submit 3 or 4 dependent CLs. If there is a way to send them all to
the review tool, I'd be glad to know it. Otherwise, we'll have to do it the slow
way.

[digit, 2012-05-29 19:51:57]

On 2012/05/29 19:44:25, digit wrote:
> On 2012/05/29 19:40:10, DerekS wrote:
> > do you want us to start reviewing this or is this CL still a work in
progress?
> 
> the CL should be good by itself. However it looks it's missing a few files
> (SkUtilsArm.h, SkUtilsArm.cpp). Let me check this and update a corrected one.
> 
> My plan is to submit 3 or 4 dependent CLs. If there is a way to send them all
to
> the review tool, I'd be glad to know it. Otherwise, we'll have to do it the
slow
> way.

ok, patch set 3 contains the missing file.

Note that I've put SkUtilsArm.cpp inside src/core, and SkUtilsArm.h inside
include/core/ mainly because the header must be included by src/core/ files (as
well as some in src/opts).

If you prefer a different location, please let me know what guidelines to
follow.

[reed1, 2012-05-29 20:37:31]

some inline comments, plus a style nit: skia functions (should) place their
opening { on the same line as the function. e.g.

bool sk_cpu_arm_foo() {
...
}

http://codereview.appspot.com/6247058/diff/5001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/5001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:40: 
Why not place this function before sk_cpu_arm_probe/has_neon, so we don't need
the forward declaration?

http://codereview.appspot.com/6247058/diff/5001/src/opts/opts_check_arm.cpp
File src/opts/opts_check_arm.cpp (right):

http://codereview.appspot.com/6247058/diff/5001/src/opts/opts_check_arm.cpp#n...
src/opts/opts_check_arm.cpp:33: return NULL;
since sk_cpu_arm_has_neon() will trivially inline in the NONE and ALWAYS cases,
can we remove the #elif checks here and below?

[reed1, 2012-05-29 20:37:52]

btw -- I think SkUtilsArm.h is very clear

[digit, 2012-05-29 21:03:15]

http://codereview.appspot.com/6247058/diff/5001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/5001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:40: 
On 2012/05/29 20:37:31, reed1 wrote:
> Why not place this function before sk_cpu_arm_probe/has_neon, so we don't need
> the forward declaration?

No precise reason. I can change it.

http://codereview.appspot.com/6247058/diff/5001/src/opts/opts_check_arm.cpp
File src/opts/opts_check_arm.cpp (right):

http://codereview.appspot.com/6247058/diff/5001/src/opts/opts_check_arm.cpp#n...
src/opts/opts_check_arm.cpp:33: return NULL;
On 2012/05/29 20:37:31, reed1 wrote:
> since sk_cpu_arm_has_neon() will trivially inline in the NONE and ALWAYS
cases,
> can we remove the #elif checks here and below?

For SK_ARM_NEON_IS_NONE, the neon functions are not declared nor defined, so the
code will not compile unless we put it in its block.

We could keep it for SK_ARM_NEON_IS_ALWAYS, but I prefer to make it clear
through the #elif chain that:

- there is no NEON code path in NONE
- there are *only* NEON code paths in ALWAYS
- there are both code paths in DYNAMIC

This is not very important here in this specific case, but will become more
visible when changes to SkBitmapProcState_XXXX.cpp are uploaded.

[digit, 2012-05-30 11:16:22]

Ping. Any chance you can review patch 4 today? I have 3 more waiting after this
one :)

[caryclark1, 2012-05-30 12:09:36]

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:32: #    defined D(...) (fprintf(stderr,__VA_ARGS__),
fprintf(stderr, "\n"))
s/defined/define/
(which makes me wonder, can you test this with NEON_DEBUG and
!SK_BUILD_FOR_ANDROID ?)

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:32: #    defined D(...) (fprintf(stderr,__VA_ARGS__),
fprintf(stderr, "\n"))
It would be more Skia-like to use SkDebugf here.

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:50: char prop[PROP_VALUE_MAX];
where is PROP_VALUE_MAX defined?

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:57: D("%s: %s", PROP_NAME, prop);
PROP_NAME is defined for SK_BUILD_FOR_ANDROID only

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:121: char*  line = (char*) memmem(buffer, buffer_end -
buffer, "\nFeatures\t", 10);
nit: here and elsewhere, fit in 80 cols

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:121: char*  line = (char*) memmem(buffer, buffer_end -
buffer, "\nFeatures\t", 10);
const char features[] = "\nFeatures\t";
  allows sizeof(features) - 1 
below

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:148: } while (0);
I like this pattern (instead of using goto to get to the end of a block). I'll
start using it myself!

[DerekS, 2012-05-30 12:20:44]

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:30: #    define D(...) 
__android_log_print(ANDROID_LOG_INFO, "skia", __VA_ARGS__)
In addition to below you can also use SkDebugf here which will eliminate the
need for the android specific includes.

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:41: sk_cpu_arm_check_neon(void)
style nit: this signature should be on one line if it is under 80 characters.

[digit, 2012-05-30 12:32:35]

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:30: #    define D(...) 
__android_log_print(ANDROID_LOG_INFO, "skia", __VA_ARGS__)
On 2012/05/30 12:20:44, DerekS wrote:
> In addition to below you can also use SkDebugf here which will eliminate the
> need for the android specific includes.
ok

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:32: #    defined D(...) (fprintf(stderr,__VA_ARGS__),
fprintf(stderr, "\n"))
Thanks. Unfortunately, I don't have a non-Android ARM-based Linux system to test
this on (or the corresponding toolchain).

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:41: sk_cpu_arm_check_neon(void)
On 2012/05/30 12:20:44, DerekS wrote:
> style nit: this signature should be on one line if it is under 80 characters.

oh. ok, no problem

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:50: char prop[PROP_VALUE_MAX];
On 2012/05/30 12:09:37, caryclark1 wrote:
> where is PROP_VALUE_MAX defined?

it is defined by <sys/system_properties.h> on Android

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:57: D("%s: %s", PROP_NAME, prop);
On 2012/05/30 12:09:37, caryclark1 wrote:
> PROP_NAME is defined for SK_BUILD_FOR_ANDROID only
good catch

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:121: char*  line = (char*) memmem(buffer, buffer_end -
buffer, "\nFeatures\t", 10);
On 2012/05/30 12:09:37, caryclark1 wrote:
> const char features[] = "\nFeatures\t";
>   allows sizeof(features) - 1 
> below
ok, will do.

[digit, 2012-05-30 12:46:04]

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9001/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:169: static bool sHasArmNeon;
Damn, the line above is a left-over from a previous version of the code. Due to
it, the implementation always returned false even if the log showed that the
device supports NEON.

I'll fix this.

[digit, 2012-05-30 13:05:43]

And here comes Patch 5. Please have a look. Thanks.

[DerekS, 2012-05-30 13:15:26]

http://codereview.appspot.com/6247058/diff/9002/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9002/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:47: #  else         
403db1eb1e586509e8b2c4feae80670759d565f1
I don't think you intended to add this hash here.

[digit, 2012-05-30 13:19:47]

Thanks, a new patch has been uploaded. Surprisingly the old one compiled without
complaints, even with the hash :-(

[digit, 2012-05-30 13:19:55]

http://codereview.appspot.com/6247058/diff/9002/src/core/SkUtilsArm.cpp
File src/core/SkUtilsArm.cpp (right):

http://codereview.appspot.com/6247058/diff/9002/src/core/SkUtilsArm.cpp#newco...
src/core/SkUtilsArm.cpp:47: #  else         
403db1eb1e586509e8b2c4feae80670759d565f1
On 2012/05/30 13:15:26, DerekS wrote:
> I don't think you intended to add this hash here.

Mmm, I assume gcl/git have been wonky. I'll try a reupload. Thanks because the
code compiles just fine here. :-/

[reed1, 2012-05-30 13:51:51]

would still like to see functions() { with the open-{ on the same line (in
SkUtilsArm.cpp) but lgtm

[digit, 2012-05-30 13:52:23]

On 2012/05/30 13:51:51, reed1 wrote:
> would still like to see functions() { with the open-{ on the same line (in
> SkUtilsArm.cpp) but lgtm

ok, I'll fix this in the next patch then. Thanks

[caryclark1, 2012-05-30 13:57:06]

lgtm

[DerekS, 2012-05-30 14:03:46]

lgtm