Bug 79441 – IPP optimization for functions vsma, vsmul, vadd, vmul and vmaxmgv

• Component: Web Audio
• Status: RESOLVED CONFIGURATION CHANGED

[Gao Chun]

Implement VectorMath::vsma(), VectorMath::vsmul(), VectorMath::vadd(), VectorMath::vmul() and VectorMath::vmaxmgv() with Intel Integrated Performance Primitives(IPP).

[Gao Chun]

Created attachment 128648 [details]
Implement VectorMath::vsma(), VectorMath::vsmul(), VectorMath::vadd(), VectorMath::vmul() and VectorMath::vmaxmgv() with Intel Integrated Performance Primitives(IPP).

[Raymond Toy]

(In reply to comment #1)
> Created an attachment (id=128648) [details]
> Implement VectorMath::vsma(), VectorMath::vsmul(), VectorMath::vadd(), VectorMath::vmul() and VectorMath::vmaxmgv() with Intel Integrated Performance Primitives(IPP).

I have very briefly looked at the changes, which seem ok.  I will give a closer look soon.

However, there needs to be a change log, and I'd like to see some numbers on how much faster IPP makes these routines compared to either the SSE2 versions or the default versions.

Also, it might be better to review this after bug 77950 (sse2 for vsvesq and vmaxmgv) has been committed.

[Alexey Proskuryakov]

USE(WEBAUDIO_IPP) looks like dead code, which we generally don't allow in WebKit.

Platform.h should have code to enable it on some platforms.

[Gao Chun]

Thanks for your comment, it seems there are some conflicts between this patch and patches for bug 77950(sse2 for vsvesq and vmaxmgv), I will resubmit a new patch with change log and performance test report after bug 77950  has been committed.

> > Created an attachment (id=128648) [details] [details]
> > Implement VectorMath::vsma(), VectorMath::vsmul(), VectorMath::vadd(), VectorMath::vmul() and VectorMath::vmaxmgv() with Intel Integrated Performance Primitives(IPP).
> 
> I have very briefly looked at the changes, which seem ok.  I will give a closer look soon.
> 
> However, there needs to be a change log, and I'd like to see some numbers on how much faster IPP makes these routines compared to either the SSE2 versions or the default versions.
> 
> Also, it might be better to review this after bug 77950 (sse2 for vsvesq and vmaxmgv) has been committed.

[Gao Chun]

Created attachment 130336 [details]
Patch

[Raymond Toy]

Comment on attachment 130336 [details]
Patch

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

Looks good overall.  Just a few comments....

> Source/WebCore/ChangeLog:5
> +

Can you give some information on how much faster these routines are with IPP compared to the SSE2 versions?

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Can't commit this with "OOPS!".  Do you know if these functions are tested by existing layout tests?  If so, you can just say they are covered by existing tests.

> Source/WebCore/platform/audio/VectorMath.cpp:116
> +#if USE(WEBAUDIO_IPP)

Maybe combine line 114 and 116 into #elif USE(WEBAUDIO_IPP)

Also, I'm not sure which is better.  It's nice to have teh IPP implementations all in one section like the special versions for Darwin, but it might fit better if the bodies of these new routines were added the existing routines so that there would be an SSE2 version, an IPP version, and a default in each function.  But that makes each function harder to read, but it also makes it easier to see if one function needs to be updated to update all the different versions accordingly.

> Source/WebCore/platform/audio/VectorMath.cpp:117
> +// Use the highly optimized versions that implemented with IPP.

Typo.  "that implemented" -> "that are implemented"

[Gao Chun]

Created attachment 130616 [details]
Patch

[Gao Chun]

Comment on attachment 130336 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> Can't commit this with "OOPS!".  Do you know if these functions are tested by existing layout tests?  If so, you can just say they are covered by existing tests.

It's already covered by existing layout tests.

>> Source/WebCore/platform/audio/VectorMath.cpp:116
>> +#if USE(WEBAUDIO_IPP)
> 
> Maybe combine line 114 and 116 into #elif USE(WEBAUDIO_IPP)
> 
> Also, I'm not sure which is better.  It's nice to have teh IPP implementations all in one section like the special versions for Darwin, but it might fit better if the bodies of these new routines were added the existing routines so that there would be an SSE2 version, an IPP version, and a default in each function.  But that makes each function harder to read, but it also makes it easier to see if one function needs to be updated to update all the different versions accordingly.

All new routines' bodies have been added to the existing routines, and it looks better.

[Raymond Toy]

Comment on attachment 130616 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +

Thanks for adding the numbers.  Very nice improvement!

> Source/WebCore/platform/audio/VectorMath.cpp:125
>      if ((sourceStride == 1) && (destStride == 1)) {

Since both IPP and SSE2 have a restriction on the strides, would it make sense to move the IPP code inside the if so that it is common to both?  Something like

#if defined(__SSE2__) || USE(WEBAUDIO_IPP)
if ((sourceStride == 1) && (destStride == 1)) {
#if USE(WEBAUDIO_IPP)
   ippsAddProduceC_32f(...)
#else /* SSE2 */
   /* original sse2 code */
#endif
}
#endif

Same comment applies to the rest of the IPP code.

[Gao Chun]

Created attachment 131318 [details]
Patch

[Gao Chun]

Comment on attachment 130616 [details]
Patch

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

>> Source/WebCore/platform/audio/VectorMath.cpp:125
>>      if ((sourceStride == 1) && (destStride == 1)) {
> 
> Since both IPP and SSE2 have a restriction on the strides, would it make sense to move the IPP code inside the if so that it is common to both?  Something like
> 
> #if defined(__SSE2__) || USE(WEBAUDIO_IPP)
> if ((sourceStride == 1) && (destStride == 1)) {
> #if USE(WEBAUDIO_IPP)
>    ippsAddProduceC_32f(...)
> #else /* SSE2 */
>    /* original sse2 code */
> #endif
> }
> #endif
> 
> Same comment applies to the rest of the IPP code.

Improved, please review again.

[Raymond Toy]

Comment on attachment 131318 [details]
Patch

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

> Source/WebCore/platform/audio/VectorMath.cpp:170
> +#endif

In the body for the IPP case, could we set n = 0 so the following while loop is never executed?  This would, I think, get rid of the #if/#endif here and also at line 177.  I think this simplifies the code.

> Source/WebCore/platform/audio/VectorMath.cpp:407
> +#endif

Same comment here as the comment for vsma, line 170.

> Source/WebCore/platform/audio/VectorMath.cpp:558
> +#endif

Same comment here as for vsma, line 170.

[Gao Chun]

Created attachment 131495 [details]
Patch

[Gao Chun]

Comment on attachment 131318 [details]
Patch

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

>> Source/WebCore/platform/audio/VectorMath.cpp:170
>> +#endif
> 
> In the body for the IPP case, could we set n = 0 so the following while loop is never executed?  This would, I think, get rid of the #if/#endif here and also at line 177.  I think this simplifies the code.

It's a good suggestion, I have also replaced "n = 0" with "return" to get rid all of #if/#endif in each function. Pls review again.

[Raymond Toy]

Comment on attachment 131495 [details]
Patch

I had not considered using return's.  This makes the patch simpler with fewer #if/#endif's.  Looks good.  Thanks for the patch.

[Gao Chun]

Whether the patch is OK to commit or need more review?

[Eric Seidel (no email)]

Chris?

[Brent Fulgham]

Comment on attachment 131495 [details]
Patch

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

Seems like all requested changes have been made, and look reasonable.  I found a tiny typo, but that can be fixed when this is landed.

> Source/WebCore/platform/audio/VectorMath.cpp:248
> +    if ((sourceStride1 ==1) && (sourceStride2 == 1) && (destStride == 1)) {

Nit: We need a space between the equals and the 1 here. ("==1" should be "== 1").

[Ahmad Saleem]

I checked with BugID and it seems this didn't landed. Do we need this now? Thanks!

[Kenneth Russell]

WebAudio's IPP implementation in WebKit was removed some time ago, so this patch is obsolete.