[Webkit-unassigned] [Bug 75528] Optimize the multiply-add in Biquad.cpp::process

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=75528





--- Comment #45 from xingnan.wang at intel.com  2012-03-19 22:44:34 PST ---
(From update of attachment 132532)
Hi Ray, patch is updated as your comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=132532&action=review

>> Source/WebCore/platform/audio/Biquad.cpp:113
>> +    __m128d mma1 = _mm_set_sd(-a1);
> 
> mma1 = (0, -a1)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:117
>> +        mm6 = mm1;
> 
> Add comment mm6 = (y2, x)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:118
>> +        mm1 = _mm_shuffle_pd(mm1, mm4, 0); // y2 = y1
> 
> Add comment that mm1 = (y1, x)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:119
>> +        mm5 = _mm_mul_pd(mm2, mm0); // (x1*b1 x2*b2)
> 
> Insert comma:  (x1 * b1, x2 * b2)
> Webkit style includes a space around arithmetic operators.  Do the same for next line.

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:121
>> +        mm0 = _mm_shuffle_pd(mm0, mm1, 1); // (x1 = x, x2 = x1)
> 
> Comment is confusing.  Maybe say mm0 = (x, x1)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:122
>> +        mm4 = _mm_mul_sd(mm4, mma1); // -y1*a1
> 
> Be more explicit about contents of mm4.
> 
> mm4 = (0, -y1 * a1)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:123
>> +        mm5 = _mm_add_pd(mm5, mm6); // (x1*b1-y2*a2 x2*b2+x*b0)
> 
> Insert comma, add space around operators:
> 
> (x1 * b1 - y2 * a2, x2 * b2 + x * b0)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:124
>> +        n--;
> 
> Why decrement n here?  Is this a pipeline issue?  If not, can we put the decrement back into while (n--)?

This will impact the pipeline, using "while(n--)" pulls down the performance a little, 45% to 43%.

>> Source/WebCore/platform/audio/Biquad.cpp:125
>> +        mm6 = mm5;
> 
> mm6 = (x1 * b1 - y2 * a2, x2 * b2 + x * b0)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:126
>> +        mm7 = _mm_load_ss(sourceP);
> 
> mm7 = (0, 0, 0, *sourceP)
> 
> And we're loading the next x value.

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:128
>> +        mm5 = _mm_add_pd(mm4, mm5); // (x1*b1-y2*a2 x2*b2+x*b0-y1*a1) 
> 
> Same comment as for line 123, 119.
> 
> Any advantage in doing mm_add_pd?  It looks like we don't care about the high part of mm5.  Is that right?

Yes, _mm_add_sd is better.

>> Source/WebCore/platform/audio/Biquad.cpp:129
>> +        mm6 = _mm_shuffle_pd(mm6, mm6, 1);
> 
> mm6 = (x2 * b2 + x * b0, x1 * b1 - y2 * a2)

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:130
>> +        mm5 = _mm_add_pd(mm5, mm6); // y
> 
> mm5 = (x1*b1-y2*a2+x2*b2+x*b0, y = x2*b2+x*b0-y1*a1+x1*b1-y2*a2), and we ignore high part of mm5, so maybe just mm_add_sd?

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:133
>> +        mm4 = mm5; // y1 = y
> 
> mm4 = (x1*b1-y2*a2+x2*b2+x*b0, y)

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.