[webkit-reviews] review denied: [Bug 73182] Need SSE optimization for functions vfmul and vadd : [Attachment 116909] Update the patch (Implement the SSE optimization for vsmul and vadd)

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied	review:
Bug 73182: Need SSE optimization for functions vfmul and vadd
https://bugs.webkit.org/show_bug.cgi?id=73182

Attachment 116909: Update the patch (Implement the SSE optimization for vsmul
and vadd)
https://bugs.webkit.org/attachment.cgi?id=116909&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116909&action=review


> Source/WebCore/platform/audio/VectorMath.cpp:76
> +	   // If the sourceP address does not 16-byte align, the first several
frames(at most three) should be processed seperately.

Missing space before (.

> Source/WebCore/platform/audio/VectorMath.cpp:77
> +	   while ((((size_t)sourceP & 0x0F) != 0) && n) {

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:87
> +	   __m128 *pSource, *pDest;

We prefer one declaration per line and the * goes next to the type.

> Source/WebCore/platform/audio/VectorMath.cpp:91
> +	       pSource = reinterpret_cast<__m128*>((float*)sourceP);

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:93
> +	       // There is less than 1% performance enhancement when pulling
the if statement out of the loop, so leave it here.

It is not clear why you wouldn't want the 1% performance gain here.

> Source/WebCore/platform/audio/VectorMath.cpp:94
> +	       if (((size_t)destP & 0x0F) != 0) {

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:136
> +	   while ((((size_t)source1P & 0x0F) != 0) && n) {

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:147
> +	   __m128 * pSource1, *pSource2, *pDest;
> +	   __m128 source2, dest;

We prefer one declaration per line and the * goes next to the type.

> Source/WebCore/platform/audio/VectorMath.cpp:150
> +	   bool source2Aligned = !((size_t)source2P & 0x0F);
> +	   bool destAligned = !((size_t)destP & 0x0F);

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:155
> +		   pSource1 = reinterpret_cast<__m128*>((float*)source1P);
> +		   pSource2 = reinterpret_cast<__m128*>((float*)source2P);

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:167
> +		   pSource1 = reinterpret_cast<__m128*>((float*)source1P);
> +		   pSource2 = reinterpret_cast<__m128*>((float*)source2P);

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:178
> +		   pSource1 = reinterpret_cast<__m128*>((float*)source1P);

Please use static_cast instead of C-style cast.

> Source/WebCore/platform/audio/VectorMath.cpp:188
> +	   } else if (!source2Aligned && !destAligned) // both source2 and dest
not aligned 
> +	   {

{ should go on the previous line.

> Source/WebCore/platform/audio/VectorMath.cpp:191
> +		   source2 = _mm_loadu_ps(source2P);

Please use static_cast instead of C-style cast.