4x library for NEON

4x library for NEON

CQ_EXTRA_TRYBOTS=client.skia.android:Test-Android-Nexus5-Adreno330-Arm7-Debug-Trybot

BUG=skia:

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

[msarett, 2015-03-04 15:37:36]

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

[msarett, 2015-03-04 15:37:37]

This passes existing 4x tests after a little bit of loosening of the accuracy
constraints.

[mtklein, 2015-03-04 16:48:53]

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h
File src/core/Sk4x_neon.h (right):

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode21
src/core/Sk4x_neon.h:21: template <> inline Sk4f::Sk4x(int32x4_t v) :
fVec(vcvtq_f32_s32(v)) {}
In the SSE code, we use these constructors (which are private, thankfully) for
easy bit casts (like reinterpret).  Here it looks like you're using them for
cast conversions.  Though it looks to me that you're not using them at all, so
perhaps we can just drop them?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode46
src/core/Sk4x_neon.h:46: // useful.
This SGTM.  I think this constructor should be rare in practice.  We'll mostly
be using LoadAligned().

We could even try removing this constructor in a follow up and see what stinks.

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode55
src/core/Sk4x_neon.h:55: // level if we want to get into that.
Right.  I think people typically end up writing their own vld1qa_f32().  Sounds
like a good follow up.

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode73
src/core/Sk4x_neon.h:73: M(Sk4f) divide  (const Sk4f& o) const { return
vmulq_f32(fVec, vrecpeq_f32(o.fVec)); }
TODO: how many vrecpsq_f32 should we use here?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode75
src/core/Sk4x_neon.h:75: M(Sk4f) rsqrt() const { return vrsqrteq_f32(fVec); }
TODO: how many vrsqrtsq_f32 should we use here?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode76
src/core/Sk4x_neon.h:76: M(Sk4f)  sqrt() const { return
vrecpeq_f32(vrsqrteq_f32(fVec)); }
Won't we always end up with a better answer using this->multiply(this->rsqrt())
?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode95
src/core/Sk4x_neon.h:95: // Note that XYAB and ZWCD share code.  If both are
needed, they could be
A lot of these shuffles were sort of exploratory.  It'll be hard for us to find
the intersection of useful, fast-in-SSE, and fast-in-NEON.  I'm hoping we can
evolve this over time as we find better ways to zip and shuffle.

Should be, though, that because these are all inlined, it's probably not much
worse to call XYAB() then ZWCD() than if we implemented them together.  Constant
subexpresssion elimination ought to skip the redundant work.  (I have not tested
this.)

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode152
src/core/Sk4x_neon.h:152: // TODO: There has to be a better way to implement
this.
I actually don't think there is.  movemask is a pretty unique feature of SSE.

[msarett, 2015-03-04 18:41:13]

A few follow up questions.

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h
File src/core/Sk4x_neon.h (right):

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode21
src/core/Sk4x_neon.h:21: template <> inline Sk4f::Sk4x(int32x4_t v) :
fVec(vcvtq_f32_s32(v)) {}
On 2015/03/04 16:48:53, mtklein wrote:
> In the SSE code, we use these constructors (which are private, thankfully) for
> easy bit casts (like reinterpret).  Here it looks like you're using them for
> cast conversions.  Though it looks to me that you're not using them at all, so
> perhaps we can just drop them?

Yeah I agree, I think originally, I did drop them.  If I remember correctly, the
lack of these constructors was the reason for the linking error I ran into?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode55
src/core/Sk4x_neon.h:55: // level if we want to get into that.
On 2015/03/04 16:48:52, mtklein wrote:
> Right.  I think people typically end up writing their own vld1qa_f32(). 
Sounds
> like a good follow up.

Cool I'll add a TODO.

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode73
src/core/Sk4x_neon.h:73: M(Sk4f) divide  (const Sk4f& o) const { return
vmulq_f32(fVec, vrecpeq_f32(o.fVec)); }
On 2015/03/04 16:48:53, mtklein wrote:
> TODO: how many vrecpsq_f32 should we use here?

I can't figure out how to factor out the calls to vrecpsq_f32 without adding
reciprocal as its own member function.  Do we want to do this?

https://codereview.chromium.org/975303003/diff/1/src/core/Sk4x_neon.h#newcode76
src/core/Sk4x_neon.h:76: M(Sk4f)  sqrt() const { return
vrecpeq_f32(vrsqrteq_f32(fVec)); }
On 2015/03/04 16:48:53, mtklein wrote:
> Won't we always end up with a better answer using
this->multiply(this->rsqrt())
> ?

I'm not sure exactly what you mean.  The issue that I have run into is that NEON
does not provide a sqrt instruction.  They only provide reciprocal sqrt and
reciprocal.

Also, it might be worth mentioning that I read a little more about the accuracy
of these instructions.  rsqrte and recpe provides "estimates" of these values. 
rsqrts and recps (unused) provide Newton-Raphson steps for these estimates to
converge on the true result.  If we have accuracy issues, we may need to add
these iteration steps.

[mtklein, 2015-03-04 19:16:33]

https://codereview.chromium.org/975303003/diff/20001/src/core/Sk4x_neon.h
File src/core/Sk4x_neon.h (right):

https://codereview.chromium.org/975303003/diff/20001/src/core/Sk4x_neon.h#new...
src/core/Sk4x_neon.h:21: template <> inline Sk4f::Sk4x(int32x4_t v) :
fVec(vcvtq_f32_s32(v)) {}
delete?

https://codereview.chromium.org/975303003/diff/20001/src/core/Sk4x_neon.h#new...
src/core/Sk4x_neon.h:24: template <> inline Sk4i::Sk4x(float32x4_t v) :
fVec(vcvtq_s32_f32(v)) {}
delete?

[msarett, 2015-03-04 20:27:30]

Sqrt:
0 steps: 1.996094 3.992188 4.992676 10.989258
1 steps: 1.999989 3.999977 4.999984 10.999984
2 steps: 2.000000 4.000000 5.000000 10.999999

Division:
0 steps: 0.333008 0.499023 0.599121 0.666016
1 steps: 0.333333 0.499998 0.599999 0.666666
2 steps: Exactly equal!

https://codereview.chromium.org/975303003/diff/40002/tests/Sk4xTest.cpp
File tests/Sk4xTest.cpp (right):

https://codereview.chromium.org/975303003/diff/40002/tests/Sk4xTest.cpp#newco...
tests/Sk4xTest.cpp:93: Sk4f error = roots.subtract(squares.sqrt());
I was forced to change this test.  Two iteration steps was not exactly equal.

https://codereview.chromium.org/975303003/diff/40002/tests/Sk4xTest.cpp#newco...
tests/Sk4xTest.cpp:99: REPORTER_ASSERT(r,
error.greaterThanEqual(0.0f).allTrue());
I think this is a bug in the test.

[mtklein, 2015-03-04 20:45:04]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-03-04 20:45:04]

lgtm

[commit-bot: I haz the power, 2015-03-04 20:45:39]

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

[mtklein, 2015-03-04 20:50:28]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-03-04 20:51:35]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-03-04 20:51:59]

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

[commit-bot: I haz the power, 2015-03-04 23:55:57]

Committed patchset #4 (id:40002) as
https://skia.googlesource.com/skia/+/c877a71b35bd27c46cb525c597b9a1aa006af182