[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


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84401|review?                     |review-
               Flag|                            |




--- Comment #34 from Gavin Barraclough <barraclough at apple.com>  2011-03-08 02:03:27 PST ---
(From update of attachment 84401)
Okay, I've done my best to review as neither a Neon coding nor 3D lighting expert.  I've followed the asm through the best I can, and it all seems reasonable (and rather impressively tight) to me.  The style seems good, a couple of things that I don't know whether we really have precedent for in WebKit style (e.g. the NL macro), but it all reads well to me, so I'm happy with the asm as is.

I'd like to suggest making some of the setup to this & data structures be designed to be shared with SIMD implementations for other architectures (e.g. SSE), but I don't know how helpful that would be (unclear without attempting the port exactly how much code could be shared anyway, so trying to generalize at this time may lead to wasted effort).  Refactoring as necessary would only be a small part of the task if someone does decide to port to SSE. :o)

One issue.  I don't think you should need to manually align the allocation of FELightingFloatArgumentsForNeon.  There is a macro WTF_ALIGN defined in wtf that I would hope would be able to do this for you.  You probably ought to be able to use this for s_FELightingConstantsForNeon, too.  Oh, and also, in allocating & populating neonData, I think we'd usually just use a literal initialization here (e.g.:

    FELightingPaintingDataForNeon neonData = {
        data.pixels->data(),
        data.widthDecreasedByOne - 1,
        data.heightDecreasedByOne - 1,
        0,
        0,
        0,
        floatArguments,
        s_FELightingConstantsForNeon
    };

).  Oh, and one final thing, I think a comment on 'getPowerCoefficients' to explain what it is trying to achieve wouldn't hurt. :-)  It looks to me like this method could probably be written more efficiently in integer code, using the macros from the std c libs to extract the exponent from a fp number?  I guess this isn't critical to the performance improvement in this patch, but still, the method could probably do with a little explanation.

I'm going to r- because I think you should take a quick look at the WTF_ALIGN macro & I do think getPowerCoefficients deserves a comment, but I agree that this patch looks very close to landing.

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