Replace or exclude MMX intrinsics in yuv_convert_simd_x86 due to lack of VS2010 support for them in…

Replace or exclude MMX intrinsics in yuv_convert_simd_x86 due to lack of VS2010 support for them in Win64

On Win64, exclude MMX version in ChooseFilterYUVRowsProc; still use faster
SSE2 version.
Replace _mm_empty() with new yasm that implements EmptyRegisterState_MMX()
with emms in EmptyRegisterState(), ConvertYUVToRGB32_MMX(...), and
ConvertYUVToRGB32_SSE(...).

This patch does not fix other win64 media link errors; more work remains for
bug 172938.

BUG=172938, 166496

[wolenetz, 2013-01-31 01:04:45]

Please review.
Alpha: All
Scherkus: media.gyp and the rest as you see fit

Thanks!
Matt

[scherkus (not reviewing), 2013-01-31 01:56:40]

a few cosmetic nits -- deferring to hclam@ for reviewing the more detailed asm
stuff

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
File media/base/simd/empty_register_state.h (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:5: #ifndef
MEDIA_BASE_EMPTY_REGISTER_STATE_H_
header guard is incorrect (missing SIMD)

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:10: void EmptyRegisterState_MMX(void);
a whole file for a single function seems a bit overkill ... is there an existing
file we can shove this into?

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:14: 
remove extra blank line

[Alpha Left Google, 2013-01-31 19:43:15]

Just a few nits. In general this looks good.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
File media/base/simd/convert_yuv_to_rgb_x86.cc (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb_x86.cc:41: EmptyRegisterState_MMX();
Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb_x86.cc:68: EmptyRegisterState_MMX();
Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:26: #if defined(ARCH_CPU_X86_FAMILY)
No need to do ifdef, it's just a header file.

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:50: #if !(defined(ARCH_CPU_X86_64) &&
defined(COMPILER_MSVC))
I suggest you move this block to yuv_convert.h and have something like:

#if defined(ARCH_CPU_X86_64) && defined(COMPILER_MSVC)
#define MEDIA_DO_NOT_USE_MMX_INTRINSICS
#endif

and then do a:
#if !defined(MEDIA_DO_NOT_USE_MMX_INTRINSICS)
...
#endif

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:113: EmptyRegisterState_MMX();
Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

[wolenetz, 2013-01-31 19:45:34]

On 2013/01/31 19:43:15, Alpha wrote:
> Just a few nits. In general this looks good.
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
> File media/base/simd/convert_yuv_to_rgb_x86.cc (right):
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
> media/base/simd/convert_yuv_to_rgb_x86.cc:41: EmptyRegisterState_MMX();
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
> media/base/simd/convert_yuv_to_rgb_x86.cc:68: EmptyRegisterState_MMX();
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.
> 
> https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc
> File media/base/yuv_convert.cc (right):
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
> media/base/yuv_convert.cc:26: #if defined(ARCH_CPU_X86_FAMILY)
> No need to do ifdef, it's just a header file.
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
> media/base/yuv_convert.cc:50: #if !(defined(ARCH_CPU_X86_64) &&
> defined(COMPILER_MSVC))
> I suggest you move this block to yuv_convert.h and have something like:
> 
> #if defined(ARCH_CPU_X86_64) && defined(COMPILER_MSVC)
> #define MEDIA_DO_NOT_USE_MMX_INTRINSICS
> #endif
> 
> and then do a:
> #if !defined(MEDIA_DO_NOT_USE_MMX_INTRINSICS)
> ...
> #endif
> 
>
https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
> media/base/yuv_convert.cc:113: EmptyRegisterState_MMX();
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

Ignore ps3.  Ps4 will contain the updates relative to CR comments so far.

[wolenetz, 2013-01-31 22:13:45]

Please review.

Thanks!
Matt

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
File media/base/simd/convert_yuv_to_rgb_x86.cc (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb_x86.cc:41: EmptyRegisterState_MMX();
On 2013/01/31 19:43:15, Alpha wrote:
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb_x86.cc:68: EmptyRegisterState_MMX();
On 2013/01/31 19:43:15, Alpha wrote:
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
File media/base/simd/empty_register_state.h (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:5: #ifndef
MEDIA_BASE_EMPTY_REGISTER_STATE_H_
On 2013/01/31 01:56:40, scherkus wrote:
> header guard is incorrect (missing SIMD)

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:10: void EmptyRegisterState_MMX(void);
On 2013/01/31 01:56:40, scherkus wrote:
> a whole file for a single function seems a bit overkill ... is there an
existing
> file we can shove this into?

Offline with Dale and Alpha, there is no good other file to put this into
currently.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:13: #endif  //
MEDIA_BASE_EMPTY_REGISTER_STATE_H_
Fixed header guard comment here, too.

https://codereview.chromium.org/12082087/diff/5001/media/base/simd/empty_regi...
media/base/simd/empty_register_state.h:14: 
On 2013/01/31 01:56:40, scherkus wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:26: #if defined(ARCH_CPU_X86_FAMILY)
On 2013/01/31 19:43:15, Alpha wrote:
> No need to do ifdef, it's just a header file.

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:50: #if !(defined(ARCH_CPU_X86_64) &&
defined(COMPILER_MSVC))
On 2013/01/31 19:43:15, Alpha wrote:
> I suggest you move this block to yuv_convert.h and have something like:
> 
> #if defined(ARCH_CPU_X86_64) && defined(COMPILER_MSVC)
> #define MEDIA_DO_NOT_USE_MMX_INTRINSICS
> #endif
> 
> and then do a:
> #if !defined(MEDIA_DO_NOT_USE_MMX_INTRINSICS)
> ...
> #endif

Done.

https://codereview.chromium.org/12082087/diff/5001/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:113: EmptyRegisterState_MMX();
On 2013/01/31 19:43:15, Alpha wrote:
> Use the MEDIA_DO_NOT_USE_MMX_INTRINSICS macro.

Done.

[Alpha Left Google, 2013-01-31 22:20:48]

LGTM after one nit.

https://codereview.chromium.org/12082087/diff/13001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/13001/media/base/yuv_convert.cc...
media/base/yuv_convert.cc:105: if (has_mmx)
nit: Use {} to enclose the content of this state. It's hard to read with #ifdef
and stuff.

[wolenetz, 2013-01-31 23:00:01]

Readability nit fixed.  Pending scherkus currently.

Matt

https://codereview.chromium.org/12082087/diff/13001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/13001/media/base/yuv_convert.cc...
media/base/yuv_convert.cc:105: if (has_mmx)
On 2013/01/31 22:20:48, Alpha wrote:
> nit: Use {} to enclose the content of this state. It's hard to read with
#ifdef
> and stuff.

Done.

[scherkus (not reviewing), 2013-01-31 23:55:27]

nits

as discussed offline, lets use the EmptyRegisterState() function and localize
the changes to yuv_convert.cc

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h#...
media/base/yuv_convert.h:16: // TODO(wolenetz): (crbug.com/173450) Use MMX
intrinsics when compiling win64
nit: we typically format our todos w/ the bug at the end

TODO(wolenetz): Use MMX .... http://crbug.com/173450

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h#...
media/base/yuv_convert.h:134: // This method is only used in unit test after
calling SIMD functions.
might want to update this comment ;)

[wolenetz, 2013-02-01 00:57:26]

Locally, win64 media builds without yuv link errors. Win32 and linux build media
and run media_unittests, media & http/tests/media layout tests without
regression.

Please review.

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h#...
media/base/yuv_convert.h:16: // TODO(wolenetz): (crbug.com/173450) Use MMX
intrinsics when compiling win64
On 2013/01/31 23:55:27, scherkus wrote:
> nit: we typically format our todos w/ the bug at the end
> 
> TODO(wolenetz): Use MMX .... http://crbug.com/173450

Done.

https://codereview.chromium.org/12082087/diff/10002/media/base/yuv_convert.h#...
media/base/yuv_convert.h:134: // This method is only used in unit test after
calling SIMD functions.
On 2013/01/31 23:55:27, scherkus wrote:
> might want to update this comment ;)

Done.

[Alpha Left Google, 2013-02-01 01:08:18]

LGTM.

[scherkus (not reviewing), 2013-02-01 01:13:17]

LGTM w/ a bunch of nits

https://codereview.chromium.org/12082087/diff/3009/media/base/simd/empty_regi...
File media/base/simd/empty_register_state_mmx.asm (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/simd/empty_regi...
media/base/simd/empty_register_state_mmx.asm:9: ; is not fully supported across
all toolchains.
nit: want to add TODO + mention 173450?

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:38: void EmptyRegisterState_MMX(void);
nit: we don't typically put the void type in for parameters (if this is somehow
necessary for assembly magic, feel free to ignore)

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:17: // Studio 2012? (crbug.com/173450)
pedantic nit: use "http://crbug.com/173450: without brackets or other
punctuation 'n stuff

(there are lots of random text editors and search engines that will appreciate
your easy-to-parse URL inside comments!)

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:19: #define MEDIA_DO_NOT_USE_MMX_INTRINSICS
nit: it'd be clearer to negate the define i.e., USE_MMX_INTRINSICS and have it
undefined for win64 msvc

(the !defined(MEDIA_DO_NOT_USE_MMX_INTRINSICS) makes my brain hurt)

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:20: #endif
I believe this can be moved to the .cc

[wolenetz, 2013-02-01 02:42:07]

Local Win64 shows no yuv link errors, local win32 and linux show no
media_unittests or [http/tests/]media layout test regressions.

https://codereview.chromium.org/12082087/diff/3009/media/base/simd/empty_regi...
File media/base/simd/empty_register_state_mmx.asm (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/simd/empty_regi...
media/base/simd/empty_register_state_mmx.asm:9: ; is not fully supported across
all toolchains.
On 2013/02/01 01:13:17, scherkus wrote:
> nit: want to add TODO + mention 173450?

Done.

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.cc#...
media/base/yuv_convert.cc:38: void EmptyRegisterState_MMX(void);
On 2013/02/01 01:13:17, scherkus wrote:
> nit: we don't typically put the void type in for parameters (if this is
somehow
> necessary for assembly magic, feel free to ignore)

Done.

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:17: // Studio 2012? (crbug.com/173450)
On 2013/02/01 01:13:17, scherkus wrote:
> pedantic nit: use "http://crbug.com/173450: without brackets or other
> punctuation 'n stuff
> 
> (there are lots of random text editors and search engines that will appreciate
> your easy-to-parse URL inside comments!)

Done.

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:19: #define MEDIA_DO_NOT_USE_MMX_INTRINSICS
On 2013/02/01 01:13:17, scherkus wrote:
> nit: it'd be clearer to negate the define i.e., USE_MMX_INTRINSICS and have it
> undefined for win64 msvc
> 
> (the !defined(MEDIA_DO_NOT_USE_MMX_INTRINSICS) makes my brain hurt)

Done.

https://codereview.chromium.org/12082087/diff/3009/media/base/yuv_convert.h#n...
media/base/yuv_convert.h:20: #endif
On 2013/02/01 01:13:17, scherkus wrote:
> I believe this can be moved to the .cc

Done.

[commit-bot: I haz the power, 2013-02-01 03:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/12082087/27002

[commit-bot: I haz the power, 2013-02-01 03:03:13]

Presubmit check for 12082087-27002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  media/base/simd/empty_register_state_mmx.asm

[wolenetz, 2013-02-01 07:26:15]

On 2013/02/01 03:03:13, I haz the power (commit-bot) wrote:
> Presubmit check for 12082087-27002 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Run the command: svn pset svn:eol-style LF \
>   media/base/simd/empty_register_state_mmx.asm

I will either need a committer to commit this for me, or wait until CQ is
configured to svn commit new asm files.  I've filed bug 173609 and sent a
possible CQ fix for CR: https://codereview.chromium.org/12162003/

[jschuh, 2013-02-01 14:37:10]

On 2013/02/01 07:26:15, wolenetz wrote:
> I will either need a committer to commit this for me, or wait until CQ is
> configured to svn commit new asm files.

I committed it for you as r180141.