Add MIPS SIMD Arch (MSA) optimized MirrorRow function

Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.

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

Committed: https://chromium.googlesource.com/libyuv/libyuv/+/c5323b0fdc3428b9341043e8adc2c2715a227330

[manojkumar.bhosale, 2016-08-26 10:11:51]

Description was changed from

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

R=thakis@chromium.org
BUG=
==========

to

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.
==========

[manojkumar.bhosale, 2016-08-26 10:11:51]

manojkumar.bhosale@imgtec.com changed reviewers:
+ dpranke@chromium.org, fbarchard@google.com, gordana.cmiljanovic@imgtec.com,
justincohen@chromium.org, kjellander@chromium.org

[Nico, 2016-08-26 12:14:55]

thakis@chromium.org changed reviewers:
- dpranke@chromium.org, justincohen@chromium.org, thakis@chromium.org

[Nico, 2016-08-26 12:14:56]

fbarchard is a good reviewer for libyuv changes. I'm removing me, justincohen,
dpranke from the reviewers list.

https://codereview.chromium.org/2285683002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2285683002/diff/1/BUILD.gn#newcode6
BUILD.gn:6: # in the file PATENTS. All contributing project authors may
probably want to list the .h file somewhere in here

[fbarchard1, 2016-08-31 01:20:43]

How do you build for this version?  We'll want to document that in
getting_started.md

Could you file a bug?
https://bugs.chromium.org/p/libyuv/issues/list

and then add to your description
BUG=libyuv:###
TEST=how you tested the code

As is this function wont be used.  We'll need
1. cpu_id.cc detection
2. row_any.cc support for odd widths suggested, keeping the MSA code simple/fast
for the common case. This function is mainly used for mirroring webcams in video
chat, and cameras are usually aligned sizes - 340x240, 640x480, 1280x720 or
1920x1080.
3. functions that use mirror need to be hooked up hereL
source/planar_functions.cc
source/rotate.cc
source/rotate_argb.cc
source/row_any.cc

Did you run the unittests?  Did you test performance vs the C version?

To test MSA performance:
util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose --release
--gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
ight=720 --libyuv_repeat=999 --libyuv_flags=-1"

To test C performance
util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose --release
--gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
ight=720 --libyuv_repeat=999 --libyuv_flags=1 --libyuv_cpu_info=1"


Caveat this is a simple function, but not very important.  The most important
and performance critical function is I420ToARGB()

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

https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:11: #ifndef __MACROS_MSA_H__
unconventional to have a platform specific header here.  normally it would be in
the source, or in row.h
as it applies only to row_msa.cc a better name may be row_msa.h

https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
include/libyuv/macros_msa.h:20: #define ST_B(RTYPE, in, pdst) *((RTYPE *)(pdst))
= (in)
no space before * in (RTYPE*)

https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode12
source/row_msa.cc:12: #include "libyuv/macros_msa.h"
This header wont compile on intel/arm as is.  May want to move the
defined(__mips_msa) around more of this file and/or put guards in
libyuv/macros_msa.h

https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode25
source/row_msa.cc:25: v16u8 src0, src1, src2, src3, src4, src5, src6, src7;
prefer uvec8 be defined as v16u8 in row.h if that works.

https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode28
source/row_msa.cc:28: int32_t cnt64, cnt32, cnt16;
prefer int32 defined in libyuv/basic_types.h
or if storage size is not critical, use int.

https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode44
source/row_msa.cc:44: if ((width >> 4) & 0x7)
its not necessary to hand odd widths in row functions.
Define the most efficient version here, and a MirrorRow_Any_MSA() in row_any,h

[manojkumar.bhosale, 2016-08-31 10:07:35]

On 2016/08/31 01:20:43, fbarchard1 wrote:
> How do you build for this version?  We'll want to document that in
> getting_started.md
> 
> Could you file a bug?
> https://bugs.chromium.org/p/libyuv/issues/list
> 
> and then add to your description
> BUG=libyuv:###
> TEST=how you tested the code
> 
> As is this function wont be used.  We'll need
> 1. cpu_id.cc detection
> 2. row_any.cc support for odd widths suggested, keeping the MSA code
simple/fast
> for the common case. This function is mainly used for mirroring webcams in
video
> chat, and cameras are usually aligned sizes - 340x240, 640x480, 1280x720 or
> 1920x1080.
> 3. functions that use mirror need to be hooked up hereL
> source/planar_functions.cc
> source/rotate.cc
> source/rotate_argb.cc
> source/row_any.cc
> 
> Did you run the unittests?  Did you test performance vs the C version?
> 
> To test MSA performance:
> util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose --release
> --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> ight=720 --libyuv_repeat=999 --libyuv_flags=-1"
> 
> To test C performance
> util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose --release
> --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> ight=720 --libyuv_repeat=999 --libyuv_flags=1 --libyuv_cpu_info=1"
> 
> 
> Caveat this is a simple function, but not very important.  The most important
> and performance critical function is I420ToARGB()
> 
> https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h
> File include/libyuv/macros_msa.h (right):
> 
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> include/libyuv/macros_msa.h:11: #ifndef __MACROS_MSA_H__
> unconventional to have a platform specific header here.  normally it would be
in
> the source, or in row.h
> as it applies only to row_msa.cc a better name may be row_msa.h
> 
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> include/libyuv/macros_msa.h:20: #define ST_B(RTYPE, in, pdst) *((RTYPE
*)(pdst))
> = (in)
> no space before * in (RTYPE*)
> 
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc
> File source/row_msa.cc (right):
> 
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode12
> source/row_msa.cc:12: #include "libyuv/macros_msa.h"
> This header wont compile on intel/arm as is.  May want to move the
> defined(__mips_msa) around more of this file and/or put guards in
> libyuv/macros_msa.h
> 
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode25
> source/row_msa.cc:25: v16u8 src0, src1, src2, src3, src4, src5, src6, src7;
> prefer uvec8 be defined as v16u8 in row.h if that works.
> 
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode28
> source/row_msa.cc:28: int32_t cnt64, cnt32, cnt16;
> prefer int32 defined in libyuv/basic_types.h
> or if storage size is not critical, use int.
> 
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode44
> source/row_msa.cc:44: if ((width >> 4) & 0x7)
> its not necessary to hand odd widths in row functions.
> Define the most efficient version here, and a MirrorRow_Any_MSA() in row_any,h

Thanks for your detailed review comments. I will incorporate them and submit the
patch set along with new BUG number.

[manojkumar.bhosale, 2016-09-01 06:23:08]

Description was changed from

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.
==========

to

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.

BUG=libyuv:634
==========

[manojkumar.bhosale, 2016-09-13 13:23:26]

On 2016/08/31 10:07:35, manojkumar.bhosale wrote:
> On 2016/08/31 01:20:43, fbarchard1 wrote:
> > How do you build for this version?  We'll want to document that in
> > getting_started.md
> > 
[Manoj]: Added build commands in getting_started.md

> > Could you file a bug?
> > https://bugs.chromium.org/p/libyuv/issues/list
> > 
[Manoj]: Done

> > and then add to your description
> > BUG=libyuv:###
> > TEST=how you tested the code
> > 
[Manoj]: tested the optimised function for bit match (vs existing C) in my local
standalone tests and verified that it passes.

> > As is this function wont be used.  We'll need
> > 1. cpu_id.cc detection
> > 2. row_any.cc support for odd widths suggested, keeping the MSA code
> simple/fast
> > for the common case. This function is mainly used for mirroring webcams in
> video
> > chat, and cameras are usually aligned sizes - 340x240, 640x480, 1280x720 or
> > 1920x1080.
> > 3. functions that use mirror need to be hooked up hereL
> > source/planar_functions.cc
> > source/rotate.cc
> > source/rotate_argb.cc
> > source/row_any.cc
> > 
[Manoj]: Added changes in above files except rotate_argb.cc. ARGB will follow in
subsequent patches.

> > Did you run the unittests?  Did you test performance vs the C version?
> > 
> > To test MSA performance:
> > util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose
--release
> > --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> > ight=720 --libyuv_repeat=999 --libyuv_flags=-1"
> > 
> > To test C performance
> > util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose
--release
> > --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> > ight=720 --libyuv_repeat=999 --libyuv_flags=1 --libyuv_cpu_info=1"
> > 
[Manoj]: No, could not run the unittests at this point as test runner asks for
device connected. However, the performance gain over C function is as below,
MirrorRow_MSA     – ~10x
MirrorRow_Any_MSA – ~6x

> > 
> > Caveat this is a simple function, but not very important.  The most
important
> > and performance critical function is I420ToARGB()
> > 
[Manoj]: OK, will add it in subsequent patches

> >
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h
> > File include/libyuv/macros_msa.h (right):
> > 
> >
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> > include/libyuv/macros_msa.h:11: #ifndef __MACROS_MSA_H__
> > unconventional to have a platform specific header here.  normally it would
be
> in
> > the source, or in row.h
> > as it applies only to row_msa.cc a better name may be row_msa.h
> > 
[Manoj]: This header will be used across multiple msa source files getting added
in subsequent patches and so kept the generic name to it. Also suggesting a new
platform specific path as 'include/libyuv/cpu/mips/macros_msa.h'. Please let me
know your views.

> >
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> > include/libyuv/macros_msa.h:20: #define ST_B(RTYPE, in, pdst) *((RTYPE
> *)(pdst))
> > = (in)
> > no space before * in (RTYPE*)
[Manoj]: Done 

> > 
> > https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc
> > File source/row_msa.cc (right):
> > 
> >
https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode12
> > source/row_msa.cc:12: #include "libyuv/macros_msa.h"
> > This header wont compile on intel/arm as is.  May want to move the
> > defined(__mips_msa) around more of this file and/or put guards in
> > libyuv/macros_msa.h
> > 
[Manoj]: Done 

> >
https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode25
> > source/row_msa.cc:25: v16u8 src0, src1, src2, src3, src4, src5, src6, src7;
> > prefer uvec8 be defined as v16u8 in row.h if that works.
> > 
[Manoj]: v16u8 and other types are defined in msa.h in mips toolschains. Let me
know if you want me to redefine them in row.h 


> >
https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode28
> > source/row_msa.cc:28: int32_t cnt64, cnt32, cnt16;
> > prefer int32 defined in libyuv/basic_types.h
> > or if storage size is not critical, use int.
> > 
[Manoj]: Done 

> >
https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode44
> > source/row_msa.cc:44: if ((width >> 4) & 0x7)
> > its not necessary to hand odd widths in row functions.
> > Define the most efficient version here, and a MirrorRow_Any_MSA() in
row_any,h
> 
[Manoj]: Done 

> Thanks for your detailed review comments. I will incorporate them and submit
the
> patch set along with new BUG number.


Please find my comments inline for patchset 2
Thanks

[fbarchard1, 2016-09-14 01:04:54]

For code review, prefer you click on the individual comments in the source and
reply to each.

On 2016/09/13 13:23:26, manojkumar.bhosale wrote:
> On 2016/08/31 10:07:35, manojkumar.bhosale wrote:
> > On 2016/08/31 01:20:43, fbarchard1 wrote:
> > > How do you build for this version?  We'll want to document that in
> > > getting_started.md
> > > 
> [Manoj]: Added build commands in getting_started.md
> 
> > > Could you file a bug?
> > > https://bugs.chromium.org/p/libyuv/issues/list
> > > 
> [Manoj]: Done
> 
> > > and then add to your description
> > > BUG=libyuv:###
> > > TEST=how you tested the code
> > > 
> [Manoj]: tested the optimised function for bit match (vs existing C) in my
local
> standalone tests and verified that it passes.
> 
> > > As is this function wont be used.  We'll need
> > > 1. cpu_id.cc detection
> > > 2. row_any.cc support for odd widths suggested, keeping the MSA code
> > simple/fast
> > > for the common case. This function is mainly used for mirroring webcams in
> > video
> > > chat, and cameras are usually aligned sizes - 340x240, 640x480, 1280x720
or
> > > 1920x1080.
> > > 3. functions that use mirror need to be hooked up hereL
> > > source/planar_functions.cc
> > > source/rotate.cc
> > > source/rotate_argb.cc
> > > source/row_any.cc
> > > 
> [Manoj]: Added changes in above files except rotate_argb.cc. ARGB will follow
in
> subsequent patches.
> 
> > > Did you run the unittests?  Did you test performance vs the C version?
> > > 
> > > To test MSA performance:
> > > util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose
> --release
> > > --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> > > ight=720 --libyuv_repeat=999 --libyuv_flags=-1"
> > > 
> > > To test C performance
> > > util/android/test_runner.py gtest -s yuv_unittest -t 7200 --verbose
> --release
> > > --gtest_filter=*$* -a "--libyuv_width=1280 --libyuv_he
> > > ight=720 --libyuv_repeat=999 --libyuv_flags=1 --libyuv_cpu_info=1"
> > > 
> [Manoj]: No, could not run the unittests at this point as test runner asks for
> device connected. However, the performance gain over C function is as below,
> MirrorRow_MSA     – ~10x
> MirrorRow_Any_MSA – ~6x
> 
> > > 
> > > Caveat this is a simple function, but not very important.  The most
> important
> > > and performance critical function is I420ToARGB()
> > > 
> [Manoj]: OK, will add it in subsequent patches
> 
> > >
> https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h
> > > File include/libyuv/macros_msa.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> > > include/libyuv/macros_msa.h:11: #ifndef __MACROS_MSA_H__
> > > unconventional to have a platform specific header here.  normally it would
> be
> > in
> > > the source, or in row.h
> > > as it applies only to row_msa.cc a better name may be row_msa.h
> > > 
> [Manoj]: This header will be used across multiple msa source files getting
added
> in subsequent patches and so kept the generic name to it. Also suggesting a
new
> platform specific path as 'include/libyuv/cpu/mips/macros_msa.h'. Please let
me
> know your views.
> 
> > >
> >
>
https://codereview.chromium.org/2285683002/diff/1/include/libyuv/macros_msa.h...
> > > include/libyuv/macros_msa.h:20: #define ST_B(RTYPE, in, pdst) *((RTYPE
> > *)(pdst))
> > > = (in)
> > > no space before * in (RTYPE*)
> [Manoj]: Done 
> 
> > > 
> > > https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc
> > > File source/row_msa.cc (right):
> > > 
> > >
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode12
> > > source/row_msa.cc:12: #include "libyuv/macros_msa.h"
> > > This header wont compile on intel/arm as is.  May want to move the
> > > defined(__mips_msa) around more of this file and/or put guards in
> > > libyuv/macros_msa.h
> > > 
> [Manoj]: Done 
> 
> > >
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode25
> > > source/row_msa.cc:25: v16u8 src0, src1, src2, src3, src4, src5, src6,
src7;
> > > prefer uvec8 be defined as v16u8 in row.h if that works.
> > > 
> [Manoj]: v16u8 and other types are defined in msa.h in mips toolschains. Let
me
> know if you want me to redefine them in row.h 

yes, but can be done as followup.

> 
> 
> > >
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode28
> > > source/row_msa.cc:28: int32_t cnt64, cnt32, cnt16;
> > > prefer int32 defined in libyuv/basic_types.h
> > > or if storage size is not critical, use int.
> > > 
> [Manoj]: Done 
> 
> > >
> https://codereview.chromium.org/2285683002/diff/1/source/row_msa.cc#newcode44
> > > source/row_msa.cc:44: if ((width >> 4) & 0x7)
> > > its not necessary to hand odd widths in row functions.
> > > Define the most efficient version here, and a MirrorRow_Any_MSA() in
> row_any,h
> > 
> [Manoj]: Done 
> 
> > Thanks for your detailed review comments. I will incorporate them and submit
> the
> > patch set along with new BUG number.
> 
> 
> Please find my comments inline for patchset 2
> Thanks

[fbarchard1, 2016-09-14 01:48:05]

https://codereview.chromium.org/2285683002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2285683002/diff/1/BUILD.gn#newcode6
BUILD.gn:6: # in the file PATENTS. All contributing project authors may
On 2016/08/26 12:14:56, Nico wrote:
> probably want to list the .h file somewhere in here

agreed.   add it to the sources

https://codereview.chromium.org/2285683002/diff/20001/CMakeLists.txt
File CMakeLists.txt (right):

https://codereview.chromium.org/2285683002/diff/20001/CMakeLists.txt#newcode84
CMakeLists.txt:84: ${ly_inc_dir}/libyuv/cpu/mips/macros_msa.h
prefer file be in libyuv folder.  havent done cpu specific folders for other
platforms.

https://codereview.chromium.org/2285683002/diff/20001/docs/getting_started.md
File docs/getting_started.md (right):

https://codereview.chromium.org/2285683002/diff/20001/docs/getting_started.md...
docs/getting_started.md:169: GYP_DEFINES="OS=android target_arch=mips64el
mips_msa=0" GYP_CROSSCOMPILE=1 ./gyp_libyuv
good, but caveat that we're migrating from GYP to GN

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu/mips...
File include/libyuv/cpu/mips/macros_msa.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu/mips...
include/libyuv/cpu/mips/macros_msa.h:12: #define __MACROS_MSA_H__
Could you add:

#if !defined(LIBYUV_DISABLE_MSA) && defined(__mips_msa)

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu_id.h
File include/libyuv/cpu_id.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu_id.h...
include/libyuv/cpu_id.h:45: static const int kCpuHasMSA = 0x30000;
please change to: static const int kCpuHasMSA = 0x40000;
These are flags... 1 bit per instruction set.

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/row.h#ne...
include/libyuv/row.h:376: defined(__mips_msa)
indent should be 4 for line continuations.
you could probably remove the  && defined(__mips__) since it is implied by 
defined(__mips_msa)

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc#newcod...
source/cpu_id.cc:262: #if defined(__mips__) && defined(__linux__)
this code block can be moved inside the previous
#if defined(__mips__) && defined(__linux__)
and share the 
cpu_info |= kCpuHasMIPS;

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc#newcod...
source/cpu_id.cc:264: cpu_info |= kCpuHasMSA;
this is a compile time test.  If built with MSA, but run on a cpu that doesnt
have it, can it be runtime detected and fall back on C code?

also note that this code current uses a constant that enables the DSPR2 code,
but that change should be in the .h

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:24: {
{ should be on same row as void MirrorRow_MSA()

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:33: {
{ should be on same line as for ()

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:37: VSHF_B2_UB(src7, src7, src6, src6, mask, mask, dst7,
dst6);
I wouldnt normally unroll this much.  2 or 4 vectors would normally provide a
good balance of size/performance?

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:46: if (width & 0x40)
suggest removing everything from here down to make the code simple and fastest
in the common case, which is camera and screen sizes.  e.g. 1280x720.
The MirrorRow_Any_MSA will handle the edge cases.
The current _Any implementations will call MirrorRow_MSA once in a temporary
buffer, which in this case would be 128 pixels wide.

[manojkumar.bhosale, 2016-09-14 12:45:30]

Incorporated review comments

https://codereview.chromium.org/2285683002/diff/20001/CMakeLists.txt
File CMakeLists.txt (right):

https://codereview.chromium.org/2285683002/diff/20001/CMakeLists.txt#newcode84
CMakeLists.txt:84: ${ly_inc_dir}/libyuv/cpu/mips/macros_msa.h
On 2016/09/14 01:48:04, fbarchard1 wrote:
> prefer file be in libyuv folder.  havent done cpu specific folders for other
> platforms.

OK. Moved back in include/libyuv

https://codereview.chromium.org/2285683002/diff/20001/docs/getting_started.md
File docs/getting_started.md (right):

https://codereview.chromium.org/2285683002/diff/20001/docs/getting_started.md...
docs/getting_started.md:169: GYP_DEFINES="OS=android target_arch=mips64el
mips_msa=0" GYP_CROSSCOMPILE=1 ./gyp_libyuv
On 2016/09/14 01:48:04, fbarchard1 wrote:
> good, but caveat that we're migrating from GYP to GN
>  

OK. Reverting this change

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu/mips...
File include/libyuv/cpu/mips/macros_msa.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu/mips...
include/libyuv/cpu/mips/macros_msa.h:12: #define __MACROS_MSA_H__
On 2016/09/14 01:48:04, fbarchard1 wrote:
> Could you add:
> 
> #if !defined(LIBYUV_DISABLE_MSA) && defined(__mips_msa)

Done.

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu_id.h
File include/libyuv/cpu_id.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/cpu_id.h...
include/libyuv/cpu_id.h:45: static const int kCpuHasMSA = 0x30000;
On 2016/09/14 01:48:04, fbarchard1 wrote:
> please change to: static const int kCpuHasMSA = 0x40000;
> These are flags... 1 bit per instruction set.

Done.

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2285683002/diff/20001/include/libyuv/row.h#ne...
include/libyuv/row.h:376: defined(__mips_msa)
On 2016/09/14 01:48:04, fbarchard1 wrote:
> indent should be 4 for line continuations.
> you could probably remove the  && defined(__mips__) since it is implied by 
> defined(__mips_msa)

Done.

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc#newcod...
source/cpu_id.cc:262: #if defined(__mips__) && defined(__linux__)
On 2016/09/14 01:48:04, fbarchard1 wrote:
> this code block can be moved inside the previous
> #if defined(__mips__) && defined(__linux__)
> and share the 
> cpu_info |= kCpuHasMIPS;

Done.

https://codereview.chromium.org/2285683002/diff/20001/source/cpu_id.cc#newcod...
source/cpu_id.cc:264: cpu_info |= kCpuHasMSA;
On 2016/09/14 01:48:04, fbarchard1 wrote:
> this is a compile time test.  If built with MSA, but run on a cpu that doesnt
> have it, can it be runtime detected and fall back on C code?
> 
> also note that this code current uses a constant that enables the DSPR2 code,
> but that change should be in the .h

Done.

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:24: {
On 2016/09/14 01:48:05, fbarchard1 wrote:
> { should be on same row as void MirrorRow_MSA()

Done.

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:33: {
On 2016/09/14 01:48:05, fbarchard1 wrote:
> { should be on same line as for ()

Done.

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:37: VSHF_B2_UB(src7, src7, src6, src6, mask, mask, dst7,
dst6);
On 2016/09/14 01:48:04, fbarchard1 wrote:
> I wouldnt normally unroll this much.  2 or 4 vectors would normally provide a
> good balance of size/performance?

Done with 4 vectors

https://codereview.chromium.org/2285683002/diff/20001/source/row_msa.cc#newco...
source/row_msa.cc:46: if (width & 0x40)
On 2016/09/14 01:48:04, fbarchard1 wrote:
> suggest removing everything from here down to make the code simple and fastest
> in the common case, which is camera and screen sizes.  e.g. 1280x720.
> The MirrorRow_Any_MSA will handle the edge cases.
> The current _Any implementations will call MirrorRow_MSA once in a temporary
> buffer, which in this case would be 128 pixels wide.

Done.

[fbarchard1, 2016-09-17 01:01:31]

lgtm

lgtm - just nits at this point

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/macros_m...
File include/libyuv/macros_msa.h (right):

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/macros_m...
include/libyuv/macros_msa.h:78: out0 = (RTYPE) __msa_vshf_b((v16i8) mask0,
(v16i8) in1, (v16i8) in0);  \
nit just a style thing, but I would find the source (row_msa.cc) more readable
using the __msa_vshf_b() intrinsic directly.

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/row.h#ne...
include/libyuv/row.h:366: #define HAS_COPYROW_MIPS
HAS_COPYROW_MIPS is mips but not dspr2 code.  put ifdef back?

https://codereview.chromium.org/2285683002/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:174: if(!strcmp(ase, " msa")) {
if a cpu had both dspr2 and msa, the early return above would prevent msa from
being detected.  As an easy fix, suggest reversing the order, so MSA is
preferred.

https://codereview.chromium.org/2285683002/diff/40001/source/rotate.cc
File source/rotate.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/rotate.cc#newcod...
source/rotate.cc:145: if (TestCpuFlag(kCpuHasMSA)) {
needs an alignment test now:

#if defined(HAS_MIRRORROW_MSA)
   if (TestCpuFlag(kCpuHasMSA)) {
     MirrorRow = MirrorRow_Any_MSA;
     if (IS_ALIGNED(width, 64)) {
       MirrorRow = MirrorRow_MSA;
     }
   }
#endif

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:22: #if !defined(LIBYUV_DISABLE_MSA) && defined(__mips_msa)
note this is fine for the entire file.

if theres a need to be selective, functions would sometimes have an ifdef as
well or instead.
leave this as is for now, til you have more functions and determine if you need
function level ifdefs.

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:24: int count;
indent should be 2 spaces
https://google.github.io/styleguide/cppguide.html#Spaces_vs._Tabs

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:31: for (count = 0; count < (width >> 6); count++) {
I've usually done counting by pixels

for (x = 0; x < width; x += 64) {

where the 64 would match the src -= 64 and dst += 64, making it less error prone
compared to >> 6 which is / 64.

[manojkumar.bhosale, 2016-09-19 08:07:21]

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/macros_m...
File include/libyuv/macros_msa.h (right):

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/macros_m...
include/libyuv/macros_msa.h:78: out0 = (RTYPE) __msa_vshf_b((v16i8) mask0,
(v16i8) in1, (v16i8) in0);  \
On 2016/09/17 01:01:30, fbarchard1 wrote:
> nit just a style thing, but I would find the source (row_msa.cc) more readable
> using the __msa_vshf_b() intrinsic directly.

Done styling.

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2285683002/diff/40001/include/libyuv/row.h#ne...
include/libyuv/row.h:366: #define HAS_COPYROW_MIPS
On 2016/09/17 01:01:30, fbarchard1 wrote:
> HAS_COPYROW_MIPS is mips but not dspr2 code.  put ifdef back?

Done.

https://codereview.chromium.org/2285683002/diff/40001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/cpu_id.cc#newcod...
source/cpu_id.cc:174: if(!strcmp(ase, " msa")) {
On 2016/09/17 01:01:30, fbarchard1 wrote:
> if a cpu had both dspr2 and msa, the early return above would prevent msa from
> being detected.  As an easy fix, suggest reversing the order, so MSA is
> preferred.

The 'ase' argument to the function will be either " dspr2" or " msa". So only
that string shall be compared ignoring other.
As of now used this function only for msa but separate call can be used for
dspr2 under #if defined(__mips_dspr2) at line 287.
Reversed order and strcmp check

https://codereview.chromium.org/2285683002/diff/40001/source/rotate.cc
File source/rotate.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/rotate.cc#newcod...
source/rotate.cc:145: if (TestCpuFlag(kCpuHasMSA)) {
On 2016/09/17 01:01:30, fbarchard1 wrote:
> needs an alignment test now:
> 
> #if defined(HAS_MIRRORROW_MSA)
>    if (TestCpuFlag(kCpuHasMSA)) {
>      MirrorRow = MirrorRow_Any_MSA;
>      if (IS_ALIGNED(width, 64)) {
>        MirrorRow = MirrorRow_MSA;
>      }
>    }
> #endif

Done.

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:22: #if !defined(LIBYUV_DISABLE_MSA) && defined(__mips_msa)
On 2016/09/17 01:01:30, fbarchard1 wrote:
> note this is fine for the entire file.
> 
> if theres a need to be selective, functions would sometimes have an ifdef as
> well or instead.
> leave this as is for now, til you have more functions and determine if you
need
> function level ifdefs.

Acknowledged.

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:24: int count;
On 2016/09/17 01:01:31, fbarchard1 wrote:
> indent should be 2 spaces
> https://google.github.io/styleguide/cppguide.html#Spaces_vs._Tabs

Done.

https://codereview.chromium.org/2285683002/diff/40001/source/row_msa.cc#newco...
source/row_msa.cc:31: for (count = 0; count < (width >> 6); count++) {
On 2016/09/17 01:01:31, fbarchard1 wrote:
> I've usually done counting by pixels
> 
> for (x = 0; x < width; x += 64) {
> 
> where the 64 would match the src -= 64 and dst += 64, making it less error
prone
> compared to >> 6 which is / 64.

Done.

[fbarchard1, 2016-09-19 17:32:41]

https://codereview.chromium.org/2285683002/diff/60001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/cpu_id.cc#newcod...
source/cpu_id.cc:168: FILE* f = fopen(cpuinfo_name, "r");
caveat - this fails in chrome sandbox.
On Intel there is a cpuid instruction for detecting features.
If there is something like that on mips that can detect msa, that would be
better for ensuring detection always works.

https://codereview.chromium.org/2285683002/diff/60001/source/planar_functions.cc
File source/planar_functions.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/planar_functions...
source/planar_functions.cc:409: #endif
Add Any version here too

#if defined(HAS_MIRRORROW_MSA)
  if (TestCpuFlag(kCpuHasMSA)) {
    MirrorRow = MirrorRow_Any_MSA;
  if (IS_ALIGNED(width, 64)) {
    MirrorRow = MirrorRow_MSA;
  }
}
#endif

https://codereview.chromium.org/2285683002/diff/60001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/row_msa.cc#newco...
source/row_msa.cc:32: src -= 64;
src -= 64 just before using src could cause a stall?
if so, moving src -= 64 to bottom of loop would help.  and presubtract 64 before
loop.  src += width - 64;

[manojkumar.bhosale, 2016-09-21 06:30:49]

https://codereview.chromium.org/2285683002/diff/60001/source/cpu_id.cc
File source/cpu_id.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/cpu_id.cc#newcod...
source/cpu_id.cc:168: FILE* f = fopen(cpuinfo_name, "r");
On 2016/09/19 17:32:41, fbarchard1 wrote:
> caveat - this fails in chrome sandbox.
> On Intel there is a cpuid instruction for detecting features.
> If there is something like that on mips that can detect msa, that would be
> better for ensuring detection always works.

At present, there is no such feature available for ASEs (msa, dspr2) and we may
have to rely on /proc/cpuinfo under linux.

https://codereview.chromium.org/2285683002/diff/60001/source/planar_functions.cc
File source/planar_functions.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/planar_functions...
source/planar_functions.cc:409: #endif
On 2016/09/19 17:32:41, fbarchard1 wrote:
> Add Any version here too
> 
> #if defined(HAS_MIRRORROW_MSA)
>   if (TestCpuFlag(kCpuHasMSA)) {
>     MirrorRow = MirrorRow_Any_MSA;
>   if (IS_ALIGNED(width, 64)) {
>     MirrorRow = MirrorRow_MSA;
>   }
> }
> #endif

Done.

https://codereview.chromium.org/2285683002/diff/60001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/60001/source/row_msa.cc#newco...
source/row_msa.cc:32: src -= 64;
On 2016/09/19 17:32:41, fbarchard1 wrote:
> src -= 64 just before using src could cause a stall?
> if so, moving src -= 64 to bottom of loop would help.  and presubtract 64
before
> loop.  src += width - 64;

Done.

[fbarchard1, 2016-09-21 17:51:07]

lgtm

https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions.cc
File source/planar_functions.cc (right):

https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions...
source/planar_functions.cc:408: if (IS_ALIGNED(width, 64)) {
nit - indent these 3 lines by 2.

https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc#newco...
source/row_msa.cc:29: src += (width - 64);
nit () not necessary.

src += width - 64;

[manojkumar.bhosale, 2016-09-22 05:38:49]

https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions.cc
File source/planar_functions.cc (right):

https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions...
source/planar_functions.cc:408: if (IS_ALIGNED(width, 64)) {
On 2016/09/21 17:51:06, fbarchard1 wrote:
> nit - indent these 3 lines by 2.

Done.

https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc
File source/row_msa.cc (right):

https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc#newco...
source/row_msa.cc:29: src += (width - 64);
On 2016/09/21 17:51:06, fbarchard1 wrote:
> nit () not necessary.
> 
> src += width - 64;

Done.

[fbarchard1, 2016-09-22 06:10:41]

On 2016/09/22 05:38:49, manojkumar.bhosale wrote:
>
https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions.cc
> File source/planar_functions.cc (right):
> 
>
https://codereview.chromium.org/2285683002/diff/80001/source/planar_functions...
> source/planar_functions.cc:408: if (IS_ALIGNED(width, 64)) {
> On 2016/09/21 17:51:06, fbarchard1 wrote:
> > nit - indent these 3 lines by 2.
> 
> Done.
> 
> https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc
> File source/row_msa.cc (right):
> 
>
https://codereview.chromium.org/2285683002/diff/80001/source/row_msa.cc#newco...
> source/row_msa.cc:29: src += (width - 64);
> On 2016/09/21 17:51:06, fbarchard1 wrote:
> > nit () not necessary.
> > 
> > src += width - 64;
> 
> Done.

lgtm

[manojkumar.bhosale, 2016-09-22 13:47:55]

manojkumar.bhosale@imgtec.com changed reviewers:
- gordana.cmiljanovic@imgtec.com

[fbarchard1, 2016-09-22 19:02:07]

On 2016/09/22 13:47:56, manojkumar.bhosale wrote:

do you need me to commit it?

[fbarchard1, 2016-09-22 23:12:25]

Description was changed from

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.

BUG=libyuv:634
==========

to

==========
Add MIPS SIMD Arch (MSA) optimized MirrorRow function

As per the preparation patch added in Chromium sources at,
2150943003: Add MIPS SIMD Arch (MSA) build flags for GYP/GN builds

This patch adds first MSA optimized function in libYUV project.

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

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/c5323b0fdc3428b9341043e8adc...
==========

[fbarchard1, 2016-09-22 23:12:26]

Committed patchset #7 (id:120001) manually as
c5323b0fdc3428b9341043e8adc2c2715a227330 (presubmit successful).