Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

R=fbarchard@google.com
BUG=libyuv:634
Performance Gain (vs C vectorized)
TransposeWx8_MSA          - ~2.7x
TransposeWx8_Any_MSA      - ~2.1x
TransposeUVWx8_MSA        - ~2.5x
TransposeUVWx8_Any_MSA    - ~2.7x

Performance Gain (vs C non-vectorized)
TransposeWx8_MSA          - ~4.6x
TransposeWx8_Any_MSA      - ~2.9x
TransposeUVWx8_MSA        - ~4.4x
TransposeUVWx8_Any_MSA    - ~3.7x

Committed: https://chromium.googlesource.com/libyuv/libyuv/+/6fa5e4eb780b67fe3275a529c6c0da9ea7b58cff

[manojkumar.bhosale, 2016-12-07 08:52:29]

Description was changed from

==========
Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

R=fbarchard@google.com
BUG=libyuv:634
==========

to

==========
Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

R=fbarchard@google.com
BUG=libyuv:634

Performance Gain (vs C vectorized)
TransposeWx8_MSA          - ~2.7x
TransposeWx8_Any_MSA      - ~2.1x
TransposeUVWx8_MSA        - ~2.5x
TransposeUVWx8_Any_MSA    - ~2.7x

Performance Gain (vs C non-vectorized)
TransposeWx8_MSA          - ~4.6x
TransposeWx8_Any_MSA      - ~2.9x
TransposeUVWx8_MSA        - ~4.4x
TransposeUVWx8_Any_MSA    - ~3.7x
==========

[manojkumar.bhosale, 2016-12-07 08:52:29]

manojkumar.bhosale@imgtec.com changed reviewers:
+ kjellander@chromium.org

[manojkumar.bhosale, 2016-12-07 08:52:56]

Updated with reviewers, cc list and performance gain numbers

[kjellander_chromium, 2016-12-07 09:21:21]

kjellander@chromium.org changed reviewers:
- kjellander@chromium.org

[manojkumar.bhosale, 2016-12-12 10:52:57]

On 2016/12/07 08:52:56, manojkumar.bhosale wrote:
> Updated with reviewers, cc list and performance gain numbers

Hi,
Any update on these patches?

[fbarchard1, 2016-12-12 19:59:19]

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h
File include/libyuv/macros_msa.h (right):

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:58: \
remove blank lines from macros.

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:130: asm volatile("usw  %[val_m],  %[pdst_sw_m] 
\n\t" \
remove \t ?  I've never found it necessary.

https://codereview.chromium.org/2553403002/diff/1/source/rotate.cc
File source/rotate.cc (right):

https://codereview.chromium.org/2553403002/diff/1/source/rotate.cc#newcode68
source/rotate.cc:68: if (IS_ALIGNED(width, 16)) {
missing the matching {

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
File source/rotate_msa.cc (right):

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
consider if this can be weaved back into simd registers and use single store per
vector?

[manojkumar.bhosale, 2016-12-13 08:52:23]

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h
File include/libyuv/macros_msa.h (right):

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:58: \
On 2016/12/12 19:59:18, fbarchard1 wrote:
> remove blank lines from macros.

Done.

https://codereview.chromium.org/2553403002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:130: asm volatile("usw  %[val_m],  %[pdst_sw_m] 
\n\t" \
On 2016/12/12 19:59:18, fbarchard1 wrote:
> remove \t ?  I've never found it necessary.

Done.

https://codereview.chromium.org/2553403002/diff/1/source/rotate.cc
File source/rotate.cc (right):

https://codereview.chromium.org/2553403002/diff/1/source/rotate.cc#newcode68
source/rotate.cc:68: if (IS_ALIGNED(width, 16)) {
On 2016/12/12 19:59:18, fbarchard1 wrote:
> missing the matching { 

Done.

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
File source/rotate_msa.cc (right):

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
On 2016/12/12 19:59:19, fbarchard1 wrote:
> consider if this can be weaved back into simd registers and use single store
per
> vector?

As each double word store is offset with dst_stride, not possible to form a
vector for single store.

[fbarchard1, 2016-12-13 18:32:03]

lgtm

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
File source/rotate_msa.cc (right):

https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
On 2016/12/13 08:52:23, manojkumar.bhosale wrote:
> On 2016/12/12 19:59:19, fbarchard1 wrote:
> > consider if this can be weaved back into simd registers and use single store
> per
> > vector?
> 
> As each double word store is offset with dst_stride, not possible to form a
> vector for single store.

Acknowledged.
msa can't store 8 bytes directly from a SIMD register?

[manojkumar.bhosale, 2016-12-14 06:23:20]

On 2016/12/13 18:32:03, fbarchard1 wrote:
> lgtm
> 
> https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
> File source/rotate_msa.cc (right):
> 
>
https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
> source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
> On 2016/12/13 08:52:23, manojkumar.bhosale wrote:
> > On 2016/12/12 19:59:19, fbarchard1 wrote:
> > > consider if this can be weaved back into simd registers and use single
store
> > per
> > > vector?
> > 
> > As each double word store is offset with dst_stride, not possible to form a
> > vector for single store.
> 
> Acknowledged.
> msa can't store 8 bytes directly from a SIMD register?

No, it can't.

[fbarchard1, 2016-12-14 18:40:38]

On 2016/12/14 06:23:20, manojkumar.bhosale wrote:
> On 2016/12/13 18:32:03, fbarchard1 wrote:
> > lgtm
> > 
> > https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
> > File source/rotate_msa.cc (right):
> > 
> >
>
https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
> > source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
> > On 2016/12/13 08:52:23, manojkumar.bhosale wrote:
> > > On 2016/12/12 19:59:19, fbarchard1 wrote:
> > > > consider if this can be weaved back into simd registers and use single
> store
> > > per
> > > > vector?
> > > 
> > > As each double word store is offset with dst_stride, not possible to form
a
> > > vector for single store.
> > 
> > Acknowledged.
> > msa can't store 8 bytes directly from a SIMD register?
> 
> No, it can't.

ok, so a full 16x16 transpose would work better.  Suggest you add a comment or
TODO that the function could be improved doing 16 byte writes.

[fbarchard1, 2016-12-14 18:40:45]

lgtm

[manojkumar.bhosale, 2016-12-15 04:34:53]

Description was changed from

==========
Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

R=fbarchard@google.com
BUG=libyuv:634

Performance Gain (vs C vectorized)
TransposeWx8_MSA          - ~2.7x
TransposeWx8_Any_MSA      - ~2.1x
TransposeUVWx8_MSA        - ~2.5x
TransposeUVWx8_Any_MSA    - ~2.7x

Performance Gain (vs C non-vectorized)
TransposeWx8_MSA          - ~4.6x
TransposeWx8_Any_MSA      - ~2.9x
TransposeUVWx8_MSA        - ~4.4x
TransposeUVWx8_Any_MSA    - ~3.7x
==========

to

==========
Add MSA optimized TransposeWx8_MSA and TransposeUVWx8_MSA functions

R=fbarchard@google.com
BUG=libyuv:634

Performance Gain (vs C vectorized)
TransposeWx8_MSA          - ~2.7x
TransposeWx8_Any_MSA      - ~2.1x
TransposeUVWx8_MSA        - ~2.5x
TransposeUVWx8_Any_MSA    - ~2.7x

Performance Gain (vs C non-vectorized)
TransposeWx8_MSA          - ~4.6x
TransposeWx8_Any_MSA      - ~2.9x
TransposeUVWx8_MSA        - ~4.4x
TransposeUVWx8_Any_MSA    - ~3.7x

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/6fa5e4eb780b67fe3275a529c6c...
==========

[manojkumar.bhosale, 2016-12-15 04:34:57]

Committed patchset #2 (id:20001) manually as
6fa5e4eb780b67fe3275a529c6c0da9ea7b58cff (presubmit successful).

[manojkumar.bhosale, 2016-12-20 09:36:11]

On 2016/12/14 18:40:38, fbarchard1 wrote:
> On 2016/12/14 06:23:20, manojkumar.bhosale wrote:
> > On 2016/12/13 18:32:03, fbarchard1 wrote:
> > > lgtm
> > > 
> > > https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
> > > File source/rotate_msa.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
> > > source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
> > > On 2016/12/13 08:52:23, manojkumar.bhosale wrote:
> > > > On 2016/12/12 19:59:19, fbarchard1 wrote:
> > > > > consider if this can be weaved back into simd registers and use single
> > store
> > > > per
> > > > > vector?
> > > > 
> > > > As each double word store is offset with dst_stride, not possible to
form
> a
> > > > vector for single store.
> > > 
> > > Acknowledged.
> > > msa can't store 8 bytes directly from a SIMD register?
> > 
> > No, it can't.
> 
> ok, so a full 16x16 transpose would work better.  Suggest you add a comment or
> TODO that the function could be improved doing 16 byte writes.

Yes, 16x16 transpose would work better for MSA, however then in rotate.cc,
in function TransposePlane, we will need to change the calling from TransposeWx8
to TransposeWx16 (processing 16x16 tile)
Will this kind of MSA specific change be allowed in rotate.cc (generic C code)?

[fbarchard1, 2016-12-20 19:12:01]

On 2016/12/20 09:36:11, manojkumar.bhosale wrote:
> On 2016/12/14 18:40:38, fbarchard1 wrote:
> > On 2016/12/14 06:23:20, manojkumar.bhosale wrote:
> > > On 2016/12/13 18:32:03, fbarchard1 wrote:
> > > > lgtm
> > > > 
> > > > https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc
> > > > File source/rotate_msa.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2553403002/diff/1/source/rotate_msa.cc#newcode67
> > > > source/rotate_msa.cc:67: val0 = __msa_copy_s_d((v2i64)dst0, 0);
> > > > On 2016/12/13 08:52:23, manojkumar.bhosale wrote:
> > > > > On 2016/12/12 19:59:19, fbarchard1 wrote:
> > > > > > consider if this can be weaved back into simd registers and use
single
> > > store
> > > > > per
> > > > > > vector?
> > > > > 
> > > > > As each double word store is offset with dst_stride, not possible to
> form
> > a
> > > > > vector for single store.
> > > > 
> > > > Acknowledged.
> > > > msa can't store 8 bytes directly from a SIMD register?
> > > 
> > > No, it can't.
> > 
> > ok, so a full 16x16 transpose would work better.  Suggest you add a comment
or
> > TODO that the function could be improved doing 16 byte writes.
> 
> Yes, 16x16 transpose would work better for MSA, however then in rotate.cc,
> in function TransposePlane, we will need to change the calling from
TransposeWx8
> to TransposeWx16 (processing 16x16 tile)
> Will this kind of MSA specific change be allowed in rotate.cc (generic C
code)?

yes.
We could either do it for all platforms, or have a block size constant for each
platform.