[webkit-reviews] review canceled: [Bug 90949] Optimizing blend filter to ARM-NEON with intrinsics : [Attachment 151874] patch

[bugzilla-daemon at webkit.org]

Gabor Rapcsanyi <rgabor at webkit.org> has canceled Gabor Rapcsanyi
<rgabor at webkit.org>'s request for review:
Bug 90949: Optimizing blend filter to ARM-NEON with intrinsics
https://bugs.webkit.org/show_bug.cgi?id=90949

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

------- Additional Comments from Gabor Rapcsanyi <rgabor at webkit.org>
(In reply to comment #2)
> (From update of attachment 151874 [details])
> Nice patch! Few comments:
> 
> View in context:
https://bugs.webkit.org/attachment.cgi?id=151874&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +	     The feBlend SVG filter modes can be greatly fasten up with
ARM-NEON since
> > +	     we are able to calculate with 2 pixels (8 channels) at the same
time.
> > +	     The code is written with NEON intrinsics and it doesn't affect the

> > +	     general - it has the same behaviour as the original algorithm.
> 
> Please mention the speedup of this patch somewhere.
> 

With this NEON optimization the blending is ~4.5 times faster for each mode.

> > Source/WebCore/platform/graphics/filters/FEBlend.cpp:159
> > +	 if (pixelArrayLength > 4)
> > +	     platformApplyNEON(srcPixelArrayA, srcPixelArrayB, dstPixelArray,
pixelArrayLength);
> > +	 else
> > +	     platformApplyGeneric(srcPixelArrayA, srcPixelArrayB,
dstPixelArray, pixelArrayLength);
> 
> I think a single pixel case is rare, and would not worth to include the
object code of the generic just because of this. It would be better to create
an array with 2 pixels, copy there the one pixel, perform the algorithm and
copy back the result.
> 

Ok I solved that case with platformApplyNEON as well.

> > Source/WebCore/platform/graphics/filters/arm/FEBlendNEON.h:39
> > +static inline uint16x8_t div255(uint16x8_t num, uint16x8_t
sixteenConst255, uint16x8_t sixteenConstOne)
> 
> I don't like introducing global symbols like these. Can\t we simply move them
inside the body of the function or define them as part of FEBlend?

I wrapped the div255 and the others into FEBlendUtilitiesNEON class so they are
not global symbols anymore.