Add support for NEON intrinsics to speed up texture compression. We can

Add support for NEON intrinsics to speed up texture compression. We can
now convert the time that we would have spent uploading the texture to
compressing it giving a net 50% memory savings for these things.

Committed: https://skia.googlesource.com/skia/+/bc9205be0a1094e312da098348601398c210dc5a

Committed: https://skia.googlesource.com/skia/+/630598cbb87edda47aa26bc7b7f93865b34cd8de

[krajcevski, 2014-07-11 14:56:54]

Uncompressed timings:

loops	min	median	mean	max	stddev	config	bench
Device supports ARM NEON instructions!
1	79.8ms	80.1ms	80.2ms	83.6ms	1%	8888	polygon
1	137ms	138ms	138ms	138ms	0%	gpu	polygon



Compressed timings:

loops	min	median	mean	max	stddev	config	bench
Device supports ARM NEON instructions!
1	79.7ms	80.1ms	80.1ms	81ms	0%	8888	polygon
1	136ms	138ms	138ms	139ms	0%	gpu	polygon

[robertphillips, 2014-07-11 15:17:47]

The parts I understood 1gtm but I defer to Mike on this.

Mike do you think Kevin should also review this?

[mtklein, 2014-07-11 15:24:27]

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
File src/opts/SkTextureCompression_opts_neon.cpp (right):

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:149: static void
compress_r11eac_blocks(uint64_t** dst, const uint8_t* src, int rowBytes) {
Seems like you don't need the double pointer indirection for dst.

uint64_t* dst1 = dst + 0;
uint64_t* dst2 = dst + 1;
...
dst +=4;

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:162: const uint8x16_t alphaRow1 =
vld1q_u8(src1);
Seems like we can't do this without knowing src is 128-bit aligned.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:227: // Since we're going to operate
on 4 blocks at a time, the src width
Is it worth falling back to non-vectorized code for widths that aren't a
multiple of 16?

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:239: uint64_t* encPtr =
reinterpret_cast<uint64_t*>(dst);
Don't we also need to check that these are aligned, or ramp up until they are? 
As far as I know, ARM chips are sort of bad with unaligned reads and writes, in
that they'll crash if you're lucky, or silently do nothing if you aren't.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:251:
SkTextureCompressorGetPlatformProc(SkTextureCompressor::Format fmt) {
Seems like we should also pass the source color type and check that inside here.
 This thing's really got two parameters, even if one is always A8 for now.

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (left):

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:615: t = (x ^ (x >> 6)) & 0xFC0000ULL;
Removing this was intentional?

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (right):

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:772: CompressionProcTable() {
We try to avoid static initializers when possible.  Chrome hates them.

Can you just write this out as a switch or if in CompressBufferToFormat rather
than creating the table ahead of time like this?  Something like,

CompressionProc proc = NULL;
if (kAlpha_8_SkColorType == srcColorType) {
  proc = SkTextureCompressorGetPlatformProc(format)
  if (NULL == proc) {
    switch (format) {
      case kLATC_Format:    proc = compress_a8_to_latc;   break;
      case kR11_EAC_Format: proc = compress_a8_to_r11eac; break;
    }
  }
}
return NULL == proc ? false : proc(dst, src, width, height, rowBytes);

[krajcevski, 2014-07-11 16:11:48]

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
File src/opts/SkTextureCompression_opts_neon.cpp (right):

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:149: static void
compress_r11eac_blocks(uint64_t** dst, const uint8_t* src, int rowBytes) {
On 2014/07/11 15:24:27, mtklein wrote:
> Seems like you don't need the double pointer indirection for dst.
> 
> uint64_t* dst1 = dst + 0;
> uint64_t* dst2 = dst + 1;
> ...
> dst +=4;

Done.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:162: const uint8x16_t alphaRow1 =
vld1q_u8(src1);
We don't need to check. The ARM intrinsics create unaligned loads by default.
That being said, there might be room for performance improvements if we do get
an aligned buffer to use the aligned load instruction to avoid an extra DWORD
per load.

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0344e/Cihejdic...
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CJAJIIGG...

On 2014/07/11 15:24:27, mtklein wrote:
> Seems like we can't do this without knowing src is 128-bit aligned.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:227: // Since we're going to operate
on 4 blocks at a time, the src width
On 2014/07/11 15:24:27, mtklein wrote:
> Is it worth falling back to non-vectorized code for widths that aren't a
> multiple of 16?

Done.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:239: uint64_t* encPtr =
reinterpret_cast<uint64_t*>(dst);
See previous comment on alignment.

On 2014/07/11 15:24:27, mtklein wrote:
> Don't we also need to check that these are aligned, or ramp up until they are?

> As far as I know, ARM chips are sort of bad with unaligned reads and writes,
in
> that they'll crash if you're lucky, or silently do nothing if you aren't.

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:251:
SkTextureCompressorGetPlatformProc(SkTextureCompressor::Format fmt) {
On 2014/07/11 15:24:27, mtklein wrote:
> Seems like we should also pass the source color type and check that inside
here.
>  This thing's really got two parameters, even if one is always A8 for now.

Done.

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (left):

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:615: t = (x ^ (x >> 6)) & 0xFC0000ULL;
On 2014/07/11 15:24:27, mtklein wrote:
> Removing this was intentional?

Yes, it was the same as line 595 above, which I moved out of the #ifdef.

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
File src/utils/SkTextureCompressor.cpp (right):

https://codereview.chromium.org/390453002/diff/1/src/utils/SkTextureCompresso...
src/utils/SkTextureCompressor.cpp:772: CompressionProcTable() {
On 2014/07/11 15:24:27, mtklein wrote:
> We try to avoid static initializers when possible.  Chrome hates them.
> 
> Can you just write this out as a switch or if in CompressBufferToFormat rather
> than creating the table ahead of time like this?  Something like,
> 
> CompressionProc proc = NULL;
> if (kAlpha_8_SkColorType == srcColorType) {
>   proc = SkTextureCompressorGetPlatformProc(format)
>   if (NULL == proc) {
>     switch (format) {
>       case kLATC_Format:    proc = compress_a8_to_latc;   break;
>       case kR11_EAC_Format: proc = compress_a8_to_r11eac; break;
>     }
>   }
> }
> return NULL == proc ? false : proc(dst, src, width, height, rowBytes);

Done.

[krajcevski, 2014-07-11 18:14:02]

+Kevin to find any glaring inefficiencies.

[mtklein, 2014-07-11 18:21:19]

lgtm

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
File src/opts/SkTextureCompression_opts_neon.cpp (right):

https://codereview.chromium.org/390453002/diff/1/src/opts/SkTextureCompressio...
src/opts/SkTextureCompression_opts_neon.cpp:162: const uint8x16_t alphaRow1 =
vld1q_u8(src1);
Ah, neat.
On 2014/07/11 16:11:47, krajcevski wrote:
> We don't need to check. The ARM intrinsics create unaligned loads by default.
> That being said, there might be room for performance improvements if we do get
> an aligned buffer to use the aligned load instruction to avoid an extra DWORD
> per load.
> 
>
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0344e/Cihejdic...
>
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/CJAJIIGG...
> 
> On 2014/07/11 15:24:27, mtklein wrote:
> > Seems like we can't do this without knowing src is 128-bit aligned.
>

[krajcevski, 2014-07-11 18:39:17]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-07-11 18:39:30]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/krajcevski@google.com/390453002/60001

[commit-bot: I haz the power, 2014-07-11 19:12:31]

Change committed as bc9205be0a1094e312da098348601398c210dc5a

[jcgregorio, 2014-07-11 20:11:03]

On 2014/07/11 19:12:31, I haz the power (commit-bot) wrote:
> Change committed as bc9205be0a1094e312da098348601398c210dc5a

Breaking the build:

 
http://108.170.220.120:10117/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib_...

[krajcevski, 2014-07-11 20:14:31]

A revert of this CL has been created in
https://codereview.chromium.org/384053003/ by krajcevski@google.com.

The reason for reverting is: Breaking chrome..

[mtklein, 2014-07-14 18:43:30]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-07-14 18:43:56]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/krajcevski@google.com/390453002/100001

[commit-bot: I haz the power, 2014-07-14 19:00:12]

Change committed as 630598cbb87edda47aa26bc7b7f93865b34cd8de

[kevin.petit, 2014-07-15 14:37:59]

Hi,

I've had a quick look and already have a few suggestions which I'd like to
benchmark. What benchmark have you used?

[krajcevski, 2014-07-15 15:00:59]

On 2014/07/15 14:37:59, kevin.petit wrote:
> Hi,
> 
> I've had a quick look and already have a few suggestions which I'd like to
> benchmark. What benchmark have you used?

I used the polygon microbenchmark in nanobench, but this codepath is currently
turned off by default. To turn it on, you need to make the following changes to
src/gpu/GrSWMaskHelper.cpp:

@@@ -18,6 -18,6 +18,9 @@@
  // TODO: try to remove this #include
  #include "GrContext.h"

++#undef GR_COMPRESS_ALPHA_MASK
++#define GR_COMPRESS_ALPHA_MASK 1
++
  namespace {
  
  /*
@@@ -103,9 -103,9 +106,17 @@@ bool GrSWMaskHelper::init(const SkIRect
      SkIRect bounds = SkIRect::MakeWH(resultBounds.width(),
                                       resultBounds.height());
  
++#if GR_COMPRESS_ALPHA_MASK
++    int cmpWidth = (bounds.fRight + 15) & ~15;
++    int cmpHeight = (bounds.fBottom + 3) & ~3;
++    if (!fBM.allocPixels(SkImageInfo::MakeA8(cmpWidth, cmpHeight))) {
++        return false;
++    }
++#else
      if (!fBM.allocPixels(SkImageInfo::MakeA8(bounds.fRight, bounds.fBottom)))
{
          return false;
      }
++#endif
      sk_bzero(fBM.getPixels(), fBM.getSafeSize());
  
      sk_bzero(&fDraw, sizeof(fDraw));
@@@ -127,9 -127,9 +138,9 @@@ bool GrSWMaskHelper::getTexture(GrAutoS
      desc.fHeight = fBM.height();
  
  #if GR_COMPRESS_ALPHA_MASK
--    static const int kLATCBlockSize = 4;
--    if (desc.fWidth % kLATCBlockSize == 0 && desc.fHeight % kLATCBlockSize ==
0) {
--        desc.fConfig = kLATC_GrPixelConfig;
++    static const int kR11_EACBlockSize = 4;
++    if (desc.fWidth % kR11_EACBlockSize == 0 && desc.fHeight %
kR11_EACBlockSize == 0) {
++        desc.fConfig = kR11_EAC_GrPixelConfig;
      } else {
  #endif
          desc.fConfig = kAlpha_8_GrPixelConfig;
@@@ -168,8 -168,8 +179,8 @@@ void GrSWMaskHelper::toTexture(GrTextur
      desc.fConfig = texture->config();
          
      // First see if we should compress this texture before uploading.
--    if (texture->config() == kLATC_GrPixelConfig) {
--        SkTextureCompressor::Format format =
SkTextureCompressor::kLATC_Format;
++    if (texture->config() == kR11_EAC_GrPixelConfig) {
++        SkTextureCompressor::Format format =
SkTextureCompressor::kR11_EAC_Format;
          SkAutoDataUnref
latcData(SkTextureCompressor::CompressBitmapToFormat(fBM, format));
          SkASSERT(NULL != latcData);