Add back ARMv7 NEON optimization for TransformationMatrix::multiply

Add back ARMv7 NEON optimization for TransformationMatrix::multiply

Microbenchmark: Execute 1 million matrix multiplication in a for loop.

Result: (measured on Arndale Octa board - Cortex-A15)
|                            | C(ms)| NEON(ms) | Speedup |
| GCC 4.8.2                  | 1530 | 1040     | 1.47x |
| Clang 4.0.0 (trunk 287685) | 2395 | 1040     | 2.30x |

BUG=669550

[Ossy, 2016-11-29 17:27:02]

Description was changed from

==========
Add back ARMv7 NEON optimization for TransformationMatrix::multiply

Microbenchmark: Execute 1 million matrix multiplication in a for loop.

Result: (measured on Arndale Octa board - Cortex-A15)
|                            | C(ms)| NEON(ms) | Speedup |
| GCC 4.8.2                  | 1530 | 1040     | 1.47x |
| Clang 4.0.0 (trunk 287685) | 2395 | 1040     | 2.30x |

BUG=669550
==========

to

==========
Add back ARMv7 NEON optimization for TransformationMatrix::multiply

Microbenchmark: Execute 1 million matrix multiplication in a for loop.

Result: (measured on Arndale Octa board - Cortex-A15)
|                            | C(ms)| NEON(ms) | Speedup |
| GCC 4.8.2                  | 1530 | 1040     | 1.47x |
| Clang 4.0.0 (trunk 287685) | 2395 | 1040     | 2.30x |

BUG=669550
==========

[Ossy, 2016-11-29 17:27:02]

ossy.szeged@gmail.com changed reviewers:
+ esprehn@chromium.org, schenney@chromium.org

[esprehn, 2016-11-29 20:17:22]

This is great, but can we add a bunch of unit tests for the matrix multiply to
verify the assembly is the same as the C++?

Also how do we get this ported into ui/gfx and skia? We really need to start
making skia faster. I worry that all these amazing optimizations aren't used in
the GPU process or inside cc which means we're slower in the whole rest of the
pipeline.

[Ossy, 2016-11-30 11:44:42]

On 2016/11/29 20:17:22, esprehn wrote:
> This is great, but can we add a bunch of unit tests for the matrix multiply to
> verify the assembly is the same as the C++?
There is one unit test for testing only multiply, but 
unit tests for rotate use multiplication too. Of course
I tested these tests, and all passed. But it's easy to
add more unittests for multiplication.

> Also how do we get this ported into ui/gfx and skia? We really need to start
> making skia faster. I worry that all these amazing optimizations aren't used
in
> the GPU process or inside cc which means we're slower in the whole rest of the
> pipeline.
I haven't checked skia yet, but I'm interested in making 
similar optimizations anywhere in the near future.

[suzyh_UTC10 (ex-contributor), 2016-12-01 03:10:25]

On 2016/11/30 at 11:44:42, ossy.szeged wrote:
> On 2016/11/29 20:17:22, esprehn wrote:
> > This is great, but can we add a bunch of unit tests for the matrix multiply
to
> > verify the assembly is the same as the C++?
> There is one unit test for testing only multiply, but 
> unit tests for rotate use multiplication too. Of course
> I tested these tests, and all passed. But it's easy to
> add more unittests for multiplication.

The concern I had (that led to removing the code originally) is that although
the test exists, it wasn't being run on any bots with this architecture, and so
this code path was untested.

Discussion on blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Z_ZPn1_Q5Z0/...
Bug context for the removal:
https://bugs.chromium.org/p/chromium/issues/detail?id=477892

Perhaps mtklein will have thoughts on this.


> > Also how do we get this ported into ui/gfx and skia? We really need to start
> > making skia faster. I worry that all these amazing optimizations aren't used
in
> > the GPU process or inside cc which means we're slower in the whole rest of
the
> > pipeline.
> I haven't checked skia yet, but I'm interested in making 
> similar optimizations anywhere in the near future.

[mtklein_C, 2016-12-02 13:24:20]

On 2016/12/01 at 03:10:25, suzyh wrote:
> On 2016/11/30 at 11:44:42, ossy.szeged wrote:
> > On 2016/11/29 20:17:22, esprehn wrote:
> > > This is great, but can we add a bunch of unit tests for the matrix
multiply to
> > > verify the assembly is the same as the C++?
> > There is one unit test for testing only multiply, but 
> > unit tests for rotate use multiplication too. Of course
> > I tested these tests, and all passed. But it's easy to
> > add more unittests for multiplication.
> 
> The concern I had (that led to removing the code originally) is that although
the test exists, it wasn't being run on any bots with this architecture, and so
this code path was untested.
> 
> Discussion on blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Z_ZPn1_Q5Z0/...
> Bug context for the removal:
https://bugs.chromium.org/p/chromium/issues/detail?id=477892
> 
> Perhaps mtklein will have thoughts on this.
> 
> 
> > > Also how do we get this ported into ui/gfx and skia? We really need to
start
> > > making skia faster. I worry that all these amazing optimizations aren't
used in
> > > the GPU process or inside cc which means we're slower in the whole rest of
the
> > > pipeline.
> > I haven't checked skia yet, but I'm interested in making 
> > similar optimizations anywhere in the near future.

Skia doesn't generally multiply 4x4 matrices in hot code paths.  There's the
occasional matrix multiply or invert, sure, but most of the time when a matrix
isn't 3x3, the main mode of operation is to  multiply a bunch of vectors by that
matrix.  The matrices tend to not actually be 4x4.  They're usually 2x3 or 3x3
2D transforms, or 3x4 or 4x5 color space transforms.  We've done a basic job of
optimizing matrix-vector multiplies for all these on important platforms
(vectorized 4- or 8- way, using FMA where possible, etc.), and we've got many
special-cased 3x3 matrix-matrix multiplies, mostly for maintaining the transform
stack in SkCanvas.

Full 3D 4x4 tends not to be a common thing in Skia, but I'd be very happy to eat
my words if we find a place that makes a big difference.  Every time we've
looked at the benefit in the last couple years, we've concluded that did make
things 2x faster, but those things only took relatively epsilon time to begin
with... we'd rather just stick to portable code for unimportant code paths.

Skia will also no longer accept assembly code except in very unusual
circumstances (like, the compiler shipped intrinsic headers that accidentally
left out an instruction).  We've found assembly difficult to maintain.  Instead,
we build this sort of thing out of vector types that zero-cost wrap each
platform's vector intrinsics: Sk4f wraps __m128 or float32x4_t, Sk4f::operator*
wraps _mm_mul_ps or vmulq_f32, etc.

[Ossy, 2016-12-12 09:52:39]

On 2016/12/01 03:10:25, suzyh wrote:
...
> The concern I had (that led to removing the code originally) is that although
> the test exists, it wasn't being run on any bots with this architecture, and
so
> this code path was untested.

You are right, this code path isn't tested on public Chromium tester bots,
similar to the ARM64 and MIPS code paths for the same function, but the
SIMD optimization were added for that platforms.

I'm not sure if ARM64 bots don't run unit tests, but I'm sure that they don't
run 
webkit tests, which contains some TransformationMatrix related tests, which must
be 
failed before https://bugs.chromium.org/p/chromium/issues/detail?id=671173

Furthermore there are only ARMv7 based Chromebooks shipped, no ARMv8 yet, so
ARMv7 
platform is still important, there is no reason not to optimize hot paths on it.

> Discussion on blink-dev:
>
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Z_ZPn1_Q5Z0/...
> Bug context for the removal:
> https://bugs.chromium.org/p/chromium/issues/detail?id=477892
> 
> Perhaps mtklein will have thoughts on this.

[Ossy, 2017-01-02 14:06:32]

ping?

[suzyh_UTC10 (ex-contributor), 2017-01-11 04:15:30]

On 2017/01/02 at 14:06:32, ossy.szeged wrote:
> ping?

Since you pinged, and haven't had a response, I'll chime in that I'm unsure that
there is test bot support, and unconvinced that this is a good idea without
that. But I am neither an actual reviewer of this CL nor an OWNER of the
relevant code, and since I am curious to see the decision here, I repeat your
ping: Ping!

[esprehn, 2017-04-07 10:31:03]

esprehn@chromium.org changed reviewers:
+ chrishtr@chromium.org
- esprehn@chromium.org

[chrishtr, 2017-04-08 18:35:41]

On 2017/01/11 at 04:15:30, suzyh wrote:
> On 2017/01/02 at 14:06:32, ossy.szeged wrote:
> > ping?
> 
> Since you pinged, and haven't had a response, I'll chime in that I'm unsure
that there is test bot support, and unconvinced that this is a good idea without
that. But I am neither an actual reviewer of this CL nor an OWNER of the
relevant code, and since I am curious to see the decision here, I repeat your
ping: Ping!

I agree with suzyh. We should not add such an optimization if is not tested
thoroughly on the bots, and via unittests.
ozzy.szeged, have you looked into or asked whether such bots could be added? Is
there an emulation mode we could consider?