[webkit-reviews] review denied: [Bug 74048] A function of vector multiply with SSE optimization is needed in VectorMath.cpp : [Attachment 118744] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied xingnan.wang at intel.com's
request for review:
Bug 74048: A function of vector multiply with SSE optimization is needed in
VectorMath.cpp
https://bugs.webkit.org/show_bug.cgi?id=74048

Attachment 118744: Patch
https://bugs.webkit.org/attachment.cgi?id=118744&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118744&action=review


> Source/WebCore/ChangeLog:12
> +	   - It`s a function for an element-by-element multiply of two float
vectors and we get 
> +	   about 3.4x performance improvement with SSE2 optimization compared
with the common 
> +	   multiply.
> +
> +	   - Use the function in
AudioBus::copyWithSampleAccurateGainValuesFrom().

You should not start the sentences by "- ". 
"It`s" -> "It's" + the subject "It" is not clear in that sentence.

> Source/WebCore/platform/audio/VectorMath.cpp:246
> +    // About 3.4x performance improvement with SSE2 optimization for the
very common case of all the strides are 1.

This should not be a comment. The commit message is enough information.

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

Coding style: "sourceStride1 ==1" -> "sourceStride1 == 1"

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

sourceP -> source1P

> Source/WebCore/platform/audio/VectorMath.cpp:268
> +#define SSE2_MULT(l, s)				  \

please use "loadInstr" and "storeInstr" instead of l, and s. Explanatory
variable names are better for readability.

> Source/WebCore/platform/audio/VectorMath.cpp:293
> +	   // Non-SSE handling for left frames which is less than 4.

" which is less than 4" in the sentence is unclear without reading the code
above. Maybe you should get rid of this part of the comment, and have a
variable for "n % 4" when you compute endP.