[webkit-reviews] review denied: [Bug 54456] Optimizing lightning filter to ARM-neon SIMD instruction set : [Attachment 84401] patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 54456: Optimizing lightning filter to ARM-neon SIMD instruction set
https://bugs.webkit.org/show_bug.cgi?id=54456

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

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.