Add SSE optimizations to SincResampler.

Add SSE optimizations to SincResampler.

These are not the same optimizations in the WebKit version of
SincResampler.  The WebKit version focuses on aligning the input
vector, resulting in at worst two unaligned loads on each kernel
index; or 2 * kKernelSize / 4 unaligned loads per call.

Instead I chose to focus on keeping the kernel vectors aligned
and eating at worst a single unaligned load on the input vector;
or kKernelSize / 4 unaligned loads per call.

Performance results from SincResamplerTest.ConvolveBenchmark:

clang version 3.2 (trunk 159409):
Convolve_C took 2100ms for 50000000 iterations.
Convolve_SSE (aligned) took 677ms for 50000000 iterations.
Convolve_SSE (unaligned) took 717ms for 50000000 iterations.

gcc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3
Convolve_C took 2183ms for 50000000 iterations.
Convolve_SSE (aligned) took 806ms for 50000000 iterations.
Convolve_SSE (unaligned) took 844ms for 50000000 iterations.

For reference, the original WebKit optimizations:
clang version 3.2 (trunk 159409):
Convolve_C took 2132ms for 50000000 iterations.
Convolve_SSE (aligned) took 1146ms for 50000000 iterations.
Convolve_SSE (unaligned) took 1797ms for 50000000 iterations.

gcc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3:
Convolve_C took 2209ms for 50000000 iterations.
Convolve_SSE (aligned) took 1450ms for 50000000 iterations.
Convolve_SSE (unaligned) took 4415ms for 50000000 iterations.

In summary, SSE provides an ~2.6x to ~3x speedup on GCC and
clang respectively.

BUG=133637
TEST=media_unittests + SincResampler/* tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149569

[DaleCurtis, 2012-07-18 03:42:19]

PTAL. You seemed most interested in this discussion at lunch, so you get to
review :)

Still needs an AlignedMemory<> setup, which I will pipe in another CL before
landing this.

[Ami GONE FROM CHROMIUM, 2012-07-18 04:40:35]

This needs either a microbenchmark or a detailed note about how to repro your
numbers (for the next time someone wants to fasterize this code).

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
File media/base/sinc_resampler.cc (right):

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:72: #define CONVOLVE_ONE_SAMPLE \
#undef this when done with it.
Except you only use this once, so why not just inline?

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:94: #define CONVOLVE_4_SAMPLES(load) \
Any reason to use a #define instead of an inline function?

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:99: static const int kFloatsPerPass =
sizeof(__m128) / sizeof(float);
Not sure what this generality buys you given you've hardcoded _4_ above
already...

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:100: static double Convolve_SSE(float *input_ptr,
float* k1, float* k2,
i can haz a test that shows the two Convolve_* functions return equal values?
(obvs., that test would only run on some machines)

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:148: // allocate with 16-byte alignment (default
linux, mac, not win)
IDK what this means

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:175: if (!convolve_proc) {
This is racy.

https://chromiumcodereview.appspot.com/10803003/diff/1/media/base/sinc_resamp...
media/base/sinc_resampler.cc:179: convolve_proc = Convolve_SSE;
Does this compile on build platforms that don't qualify for the #ifdef above?

[DaleCurtis, 2012-07-21 02:35:13]

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:72: #define CONVOLVE_ONE_SAMPLE \
On 2012/07/18 04:40:35, Ami Fischman wrote:
> #undef this when done with it.
> Except you only use this once, so why not just inline?

Good point. Leftovers from the WebKit version of the optimizations.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:94: #define CONVOLVE_4_SAMPLES(load) \
On 2012/07/18 04:40:35, Ami Fischman wrote:
> Any reason to use a #define instead of an inline function?

Not sure how that'd work since we're modifying the instruction used. In any case
I've moved this directly into each for loop for readability.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:99: static const int kFloatsPerPass =
sizeof(__m128) / sizeof(float);
On 2012/07/18 04:40:35, Ami Fischman wrote:
> Not sure what this generality buys you given you've hardcoded _4_ above
> already...

Removed.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:100: static double Convolve_SSE(float *input_ptr,
float* k1, float* k2,
On 2012/07/18 04:40:35, Ami Fischman wrote:
> i can haz a test that shows the two Convolve_* functions return equal values?
> (obvs., that test would only run on some machines)

Done.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:148: // allocate with 16-byte alignment (default
linux, mac, not win)
On 2012/07/18 04:40:35, Ami Fischman wrote:
> IDK what this means

Changed. Was a reminder to me to land my AlignedMemory CL and change this before
this lands.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:175: if (!convolve_proc) {
On 2012/07/18 04:40:35, Ami Fischman wrote:
> This is racy.

Done.

http://codereview.chromium.org/10803003/diff/1/media/base/sinc_resampler.cc#n...
media/base/sinc_resampler.cc:179: convolve_proc = Convolve_SSE;
On 2012/07/18 04:40:35, Ami Fischman wrote:
> Does this compile on build platforms that don't qualify for the #ifdef above? 

Done.

[Ami GONE FROM CHROMIUM, 2012-07-21 17:25:09]

LGTM

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.c...
media/base/sinc_resampler.cc:266: m_input = _mm_loadu_ps(input_ptr + i);
CL description says "eating at most one unaligned load"; I think you need to
clarify that :)

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.h...
media/base/sinc_resampler.h:50: #if defined(ARCH_CPU_X86_FAMILY) &&
defined(__SSE__)
#if unnecessary (fine to leave methods undefined as long as you don't use them).

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
File media/base/sinc_resampler_unittest.cc (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
media/base/sinc_resampler_unittest.cc:93: // Benchmark for the various
Convolve() methods.
Comment that this is only DISABLED_ for the benefit of bot cycle time?

Maybe a better move is to instead check it in with kConvolveIterations of 10 or
something, and let benchmarkers up that in their builds? (or make it runtime
settable, like with a flag or env var).

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
media/base/sinc_resampler_unittest.cc:136: #endif
Add an output for the relative improvement between C and SSE?

[DaleCurtis, 2012-07-24 00:13:07]

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.c...
media/base/sinc_resampler.cc:266: m_input = _mm_loadu_ps(input_ptr + i);
On 2012/07/21 17:25:09, Ami Fischman wrote:
> CL description says "eating at most one unaligned load"; I think you need to
> clarify that :)

Done.

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler.h...
media/base/sinc_resampler.h:50: #if defined(ARCH_CPU_X86_FAMILY) &&
defined(__SSE__)
On 2012/07/21 17:25:10, Ami Fischman wrote:
> #if unnecessary (fine to leave methods undefined as long as you don't use
them).

Done.

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
File media/base/sinc_resampler_unittest.cc (right):

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
media/base/sinc_resampler_unittest.cc:93: // Benchmark for the various
Convolve() methods.
On 2012/07/21 17:25:10, Ami Fischman wrote:
> Comment that this is only DISABLED_ for the benefit of bot cycle time?
> 
> Maybe a better move is to instead check it in with kConvolveIterations of 10
or
> something, and let benchmarkers up that in their builds? (or make it runtime
> settable, like with a flag or env var).

Done. --convolve-iterations is runtime settable.

http://codereview.chromium.org/10803003/diff/7004/media/base/sinc_resampler_u...
media/base/sinc_resampler_unittest.cc:136: #endif
On 2012/07/21 17:25:10, Ami Fischman wrote:
> Add an output for the relative improvement between C and SSE?

Done.

[commit-bot: I haz the power, 2012-07-24 00:18:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10803003/16001

[commit-bot: I haz the power, 2012-07-24 01:06:42]

Try job failure for 10803003-16001 (retry) on linux_rel for step
"media_unittests".
It's a second try, previously, step "media_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[DaleCurtis, 2012-07-24 01:34:02]

On 2012/07/24 01:06:42, I haz the power (commit-bot) wrote:
> Try job failure for 10803003-16001 (retry) on linux_rel for step
> "media_unittests".
> It's a second try, previously, step "media_unittests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

My aligned memory CL got reverted as well, so this is back burnered until
https://code.google.com/p/chromium/issues/detail?id=138653 is fixed.

[commit-bot: I haz the power, 2012-07-26 03:47:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10803003/4005

[commit-bot: I haz the power, 2012-07-26 05:11:16]

Try job failure for 10803003-4005 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-26 19:40:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10803003/4005

[commit-bot: I haz the power, 2012-07-26 20:04:57]

Try job failure for 10803003-4005 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-02 01:01:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10803003/22004

[commit-bot: I haz the power, 2012-08-02 01:17:29]

Try job failure for 10803003-22004 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-02 01:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10803003/22009

[commit-bot: I haz the power, 2012-08-02 02:59:47]

Change committed as 149569