Make SkPMFloats store floats in [0,255] instead of [0,1].

Make SkPMFloats store floats in [0,255] instead of [0,1].

This pushes the cost of the *255 and *1/255 conversions onto only those code
paths that need it.  We're not doing it any more efficiently than can be done
with Sk4f.

In microbenchmark isolation, this is about a 15% speedup.

BUG=skia:
NOPRESUBMIT=true

Committed: https://skia.googlesource.com/skia/+/60d2a32b2dbbaabf4a0c133c8d3ff5ad888b8e5e

[mtklein_C, 2015-03-02 22:49:16]

mtklein@chromium.org changed reviewers:
+ reed@google.com

[mtklein, 2015-03-03 02:06:52]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-03-03 02:06:52]

Oops, typo... CC'ing the real msarett...

[msarett, 2015-03-03 14:34:27]

msarett@google.com changed reviewers:
+ msarett@google.com

[msarett, 2015-03-03 14:34:28]

I added a few comments with a few ideas.

Also, I'm not sure if you are aware of this resource, but I find it extremely
useful.
https://software.intel.com/sites/landingpage/IntrinsicsGuide/

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2.h
File src/opts/SkPMFloat_SSE2.h (right):

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2....
src/opts/SkPMFloat_SSE2.h:16: _mm_store_ps(fColor, _mm_cvtepi32_ps(fix8_32));
I think we might be able to improve performance a little bit if we convert
directly from 8-bit or 16-bit ints to floats (not 100% sure because I'm having
trouble finding documentation on the latency and throughput of the applicable
instructions).

Two instructions that might be useful:
__m128 _mm_cvtpi8_ps (__m64 a) // Converts 8-bit signed integers in lower half
of vector to floats
__m128 _mm_cvtpi16_ps (__m64 a) // Converts 16-bit signed integers (there is
also an unsigned version) to floats

The 16-bit version (either signed or unsigned) would certainly be workable and
would save the 16-bit to 32-bit step.  The 8-bit version is more interesting
because it would be faster, but the issue is that there is only a signed
version, and we need unsigned.  We might write something like this:
__m64 fix8 = _mm_set_pi32(0, c);
__m128i fix8_float = _mm_cvtpi8_ps(fix8);
_mm_store_ps(fColor, fix8_float);

The issue would be that input color components in the range [128, 255] would be
converted to the range [-128, -1].  I think we could trick our way around this
by adding 128 before and after the conversion (I can draw out how this might
work).  It's not clear if this would be faster than a 16-bit version or not but
it might be worth trying.

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2....
src/opts/SkPMFloat_SSE2.h:17: SkASSERT(this->isValid());
I'm starting another comment for another train of thought.  No matter what
strategy we choose, we are going to have to pay a price for the conversion from
ints to floats (latency = 3 cycles for _mm_cvtepi32_ps).  But some of this can
be offset by the high throughput of this instruction (throughput = 1 cycle for
_mm_cvtepi32_ps).  However, because we are only converting one pixel at a time
with each call to this function, it is likely that the latency is being paid
every time and that we are not taking advantage of the throughput.  There is a
chance that if we were to convert an array of pixels in a single function call
and use a partly unrolled loop, we could see a performance improvement.

[mtklein, 2015-03-03 15:02:17]

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2.h
File src/opts/SkPMFloat_SSE2.h (right):

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2....
src/opts/SkPMFloat_SSE2.h:16: _mm_store_ps(fColor, _mm_cvtepi32_ps(fix8_32));
On 2015/03/03 14:34:28, msarett wrote:
> I think we might be able to improve performance a little bit if we convert
> directly from 8-bit or 16-bit ints to floats (not 100% sure because I'm having
> trouble finding documentation on the latency and throughput of the applicable
> instructions).
> 
> Two instructions that might be useful:
> __m128 _mm_cvtpi8_ps (__m64 a) // Converts 8-bit signed integers in lower half
> of vector to floats
> __m128 _mm_cvtpi16_ps (__m64 a) // Converts 16-bit signed integers (there is
> also an unsigned version) to floats
> 
> The 16-bit version (either signed or unsigned) would certainly be workable and
> would save the 16-bit to 32-bit step.  The 8-bit version is more interesting
> because it would be faster, but the issue is that there is only a signed
> version, and we need unsigned.  We might write something like this:
> __m64 fix8 = _mm_set_pi32(0, c);
> __m128i fix8_float = _mm_cvtpi8_ps(fix8);
> _mm_store_ps(fColor, fix8_float);
> 
> The issue would be that input color components in the range [128, 255] would
be
> converted to the range [-128, -1].  I think we could trick our way around this
> by adding 128 before and after the conversion (I can draw out how this might
> work).  It's not clear if this would be faster than a 16-bit version or not
but
> it might be worth trying.

The reason I've shied away from intrinsics like _mm_cvtpi8_ps is that they don't
compile to single instructions.
That'll be why you can't find latency and throughput numbers on them: they're
implemented as compound operations
that may vary from compiler to compiler.  Looking at my xmmintrin.h from GCC and
Clang, they do seem to vary, and they're quite a lot more work than what we've
got here.

It's certainly worth a try locally to see what code's generated and how it
performs.  I just tried myself with this code
   _mm_store_ps(fColor, _mm_cvtpu8_ps(_mm_set_pi32(0, c)));
and it ran about 4x slower (at about 4x the instructions) than my current best.

(As far as I can tell, _mm_cvtpu8_ps is what you'd want for unsigned 8 -> float
conversions.  But again, it's slow.)

As far as int->float conversions go, I really only see two options: cvtpi2ps
(_mm_cvtpi32_ps) to convert 2 at a time from SSE, or cvtepi2ps (_mm_cvtepi32_ps)
to convert 4 at a time from SSE2.  (AVX lets us go 8 at a time of course, but
that's not really realistic to target right now.)

https://codereview.chromium.org/973603002/diff/20001/src/opts/SkPMFloat_SSE2....
src/opts/SkPMFloat_SSE2.h:17: SkASSERT(this->isValid());
On 2015/03/03 14:34:28, msarett wrote:
> I'm starting another comment for another train of thought.  No matter what
> strategy we choose, we are going to have to pay a price for the conversion
from
> ints to floats (latency = 3 cycles for _mm_cvtepi32_ps).  But some of this can
> be offset by the high throughput of this instruction (throughput = 1 cycle for
> _mm_cvtepi32_ps).  However, because we are only converting one pixel at a time
> with each call to this function, it is likely that the latency is being paid
> every time and that we are not taking advantage of the throughput.  There is a
> chance that if we were to convert an array of pixels in a single function call
> and use a partly unrolled loop, we could see a performance improvement.

Yep, totally agree.  We're thinking the next logical place to go is to do
conversions of 4 pixels at a time, which also lets us do 128-bit reads and
writes of SkPMColors, and lets us get more work out of the _mm_pack
instructions.

[msarett, 2015-03-03 15:12:12]

lgtm

Wow, had no idea that some of those intrinsics compile to multiple instructions.
 That seems to defeat the purpose of using them.

I'm not sure if you've looked into this, but it looks like there is a NEON
intrinsic that does clamping for you with the convert (float32x2_t
vcvt_n_f32_u32(uint32x2_t a, __constrange(1,32) int b);).  However, their
documentation is so awful that I can't figure out how to use it yet.

[reed1, 2015-03-03 15:22:38]

api/feature lgtm

[mtklein, 2015-03-03 15:23:37]

On 2015/03/03 15:12:12, msarett wrote:
> lgtm
> 
> Wow, had no idea that some of those intrinsics compile to multiple
instructions.
>  That seems to defeat the purpose of using them.
> 
> I'm not sure if you've looked into this, but it looks like there is a NEON
> intrinsic that does clamping for you with the convert (float32x2_t
> vcvt_n_f32_u32(uint32x2_t a, __constrange(1,32) int b);).  However, their
> documentation is so awful that I can't figure out how to use it yet.

Weirdly, that instruction clamps uint32_t to float's range.  NEON vcvt
instrinsics are spelled backwards of what you'd think.  We don't need a clamp in
that direction, as we know structurally we'll be in [0,255].

[mtklein, 2015-03-03 15:23:44]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-03-03 15:24:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973603002/20001

[commit-bot: I haz the power, 2015-03-03 15:24:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 15:24:07]

Presubmit check for 973603002-20001 failed and returned exit status 1.

Traceback (most recent call last):
  File "/b/infra_internal/commit_queue/verification/presubmit_shim.py", line 34,
in <module>
    sys.exit(presubmit_support.Main(argv))
AttributeError: 'module' object has no attribute 'Main'

[rmistry, 2015-03-03 15:39:39]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2015-03-03 15:40:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973603002/20001

[commit-bot: I haz the power, 2015-03-03 15:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 15:40:06]

Presubmit check for 973603002-20001 failed and returned exit status 1.

Traceback (most recent call last):
  File "/b/infra_internal/commit_queue/verification/presubmit_shim.py", line 34,
in <module>
    sys.exit(presubmit_support.Main(argv))
AttributeError: 'module' object has no attribute 'Main'

[rmistry, 2015-03-03 15:40:32]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2015-03-03 15:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973603002/20001

[commit-bot: I haz the power, 2015-03-03 15:46:20]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/60d2a32b2dbbaabf4a0c133c8d3ff5ad888b8e5e