ARM Skia NEON patches - 23 - S32_D565_Opaque_Dither cleanup/bugfix/speed

ARM Skia NEON patches - 23 - S32_D565_Opaque_Dither cleanup/bugfix/speed




BlitRow565: S32_D565_Opaque_Dither: cleaning / bugfix

This patch brings a little code cleaning (spaces/comments) and a little
speed improvement (by using post-incrementation in the asm) but more
importantly it fixes a bug on Linux. The new code now supports ARGB
as well as ABGR.

I removed the comment as I have confirmed with benchmarks that this
code bring a *massive* (3x-7x) speedup compared to the C code.

Signed-off-by: Kévin PETIT <kevin.petit@arm.com>;

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11339

[mtklein, 2013-09-12 20:07:35]

lgtm with a request to add a small comment or two

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
File src/opts/SkBlitRow_opts_arm_neon.cpp (left):

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
src/opts/SkBlitRow_opts_arm_neon.cpp:1120: src += UNROLL;
Maybe even duplicate the same note here about why we don't need to increment?

As someone who doesn't read a lot of asm, the ! is pretty subtle.

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
File src/opts/SkBlitRow_opts_arm_neon.cpp (right):

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
src/opts/SkBlitRow_opts_arm_neon.cpp:1106: SkPMColor c = src[i-8];
Can you tack on something like
  //  The '!' in the asm block above post-incremented src by the 8 pixels it
read.
?

[kevin.petit.not.used.account, 2013-09-13 12:08:27]

Patch Set 2 addresses the comments received and does some more cleaning of the
comments in the code. I also updated the description to fix a typo and mention
the new changes.

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
File src/opts/SkBlitRow_opts_arm_neon.cpp (left):

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
src/opts/SkBlitRow_opts_arm_neon.cpp:1120: src += UNROLL;
On 2013/09/12 20:07:36, mtklein wrote:
> Maybe even duplicate the same note here about why we don't need to increment?
> 
> As someone who doesn't read a lot of asm, the ! is pretty subtle.

Done.

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
File src/opts/SkBlitRow_opts_arm_neon.cpp (right):

https://codereview.chromium.org/22269003/diff/1/src/opts/SkBlitRow_opts_arm_n...
src/opts/SkBlitRow_opts_arm_neon.cpp:1106: SkPMColor c = src[i-8];
On 2013/09/12 20:07:36, mtklein wrote:
> Can you tack on something like
>   //  The '!' in the asm block above post-incremented src by the 8 pixels it
> read.
> ?

Done.

[mtklein, 2013-09-13 12:57:35]

lgtm

[commit-bot: I haz the power, 2013-09-13 13:46:47]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit.arm@gmail.com/22269003/5001

[commit-bot: I haz the power, 2013-09-18 13:00:24]

Change committed as 11339