[webkit-reviews] review denied: [Bug 103614] Optimizing RGBA16, RGB16, ARGB16, BGRA16 unpacking functions with NEON intrinsics : [Attachment 176708] patch

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has denied Gabor Rapcsanyi
<rgabor at webkit.org>'s request for review:
Bug 103614: Optimizing RGBA16, RGB16, ARGB16, BGRA16 unpacking functions with
NEON intrinsics
https://bugs.webkit.org/show_bug.cgi?id=103614

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

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176708&action=review


> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:377
> +#if HAVE(ARM_NEON_INTRINSICS)
> +    unsigned componentsPerRow = pixelsPerRow * 4;
> +    unsigned tailComponents = componentsPerRow % 8;
> +    unsigned componentsSize = componentsPerRow - tailComponents;
> +
> +    ARM::unpackOneRowOfRGBA16LittleToRGBA8NEON(source, destination,
componentsSize);
> +
> +    source += componentsSize;
> +    destination += componentsSize;
> +    pixelsPerRow = tailComponents / 4;
> +#endif

I realized that I don't really like in this approach. The modification of the
common code path is way too big. And too ARM specific.

I would prefer:

#if HAVE(ARM_NEON_INTRINSICS) optionally other SIMDS connected with || operator

    SIMD::unpackOneRowOfRGBA16LittleToRGBA8(source, destination, pixelsPerRow)
$endif

And the SIMD class (namespace) would define the folowing interface:
inline void SIMD::unpackOneRowOfRGBA16LittleToRGBA8(const uint16_t*& source,
uint8_t*& destination, unsigned int& pixelsPerRow)

Advantages:
1) Modifications of the common code path is much shorter.
2) Can modify the arguments, since they passed as reference (usually SIMD
process a group of pixels, but not necessary all if the length is not divisible
by a certain value).
3) Can be extended to support other SIMD-s, not just NEON
4) Still it can be seen that a certain function is supported by the current CPU
(So it is not necessary to support all of these functions on all CPUs which
have SIMD support)