[Webkit-unassigned] [Bug 54456] Optimizing lightning filter to ARM-neon SIMD instruction set

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54456





--- Comment #13 from Gabor Loki <loki at webkit.org>  2011-02-21 13:19:38 PST ---
(From update of attachment 82780)
View in context: https://bugs.webkit.org/attachment.cgi?id=82780&action=review

Let me do an informal review.


> Source/WebCore/ChangeLog:8
> +        Neon is the SIMD instruction set for arm. This instruction set

Please, use capital letters for ARM.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:36
> +#if CPU(ARM) && ENABLE(ARM_NEON)

If NEON is enabled the compilers set __ARM_NEON__ define to 1. In this case, you should define the corresponding macro (ENABLE_ARM_NEON) in Platform.h.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:395
> +    char buffer[sizeof(FELightingFloatArgumentsForNeon) + 16];
> +    FELightingFloatArgumentsForNeon* floatArguments = reinterpret_cast<FELightingFloatArgumentsForNeon*>((reinterpret_cast<long>(buffer) + 15) & ~0xf);

I like magic numbers, but others don't. Please, use a static int or make a comment for this alignment trick.

> Source/WebCore/platform/graphics/filters/arm/FELightingNeonSIMD.cpp:33
> +#include "config.h"
> +#include "FELightingNeonSIMD.h"
> +
> +#if CPU(ARM) && ENABLE(ARM_NEON)
> +
> +short s_FELightingConstantsForNeon[8 * 3] __attribute__((aligned(0x10))) = {
> +    // Alpha coefficients.

Do you miss the WebCore namespace?

In JavaScriptCore/wtf/Vector.h there is a group of macros for alignment (for all platforms). Well, It may be worthwhile to move those macros to StdLibExtras.h and use it here also.

> Source/WebCore/platform/graphics/filters/arm/FELightingNeonSIMD.cpp:193
> +
> +asm (
> +".globl " TOSTRING(neonDrawLighting) NL

I feel this will fall with RVCT. I can help you in this.

> Source/WebCore/platform/graphics/filters/arm/FELightingNeonSIMD.h:45
> +#ifndef FELightingNeonSIMD_h
> +#define FELightingNeonSIMD_h
> +
> +#include <wtf/Platform.h>
> +
> +#if CPU(ARM) && ENABLE(ARM_NEON)
> +
> +// Otherwise: Distant Light.
> +#define FLAG_POINT_LIGHT                 0x01
> +#define FLAG_SPOT_LIGHT                  0x02
> +#define FLAG_CONE_EXPONENT_IS_1          0x04
> +
> +// Otherwise: Diffuse light.
> +#define FLAG_SPECULAR_LIGHT              0x10
> +#define FLAG_DIFFUSE_CONST_IS_1          0x20
> +#define FLAG_SPECULAR_EXPONENT_IS_1      0x40
> +
> +// Must be aligned to 16 bytes.
> +struct FELightingFloatArgumentsForNeon {

Why don't we put these into the WebCore namespace?

> Source/WebCore/platform/graphics/filters/arm/FELightingNeonSIMD.h:84
> +extern short s_FELightingConstantsForNeon[8 * 3];

Use static int or comment for this magic numbers as well.


Well, I need one more day to understand the assembly code, but others looks fine.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.