[webkit-reviews] review denied: [Bug 24986] ARM JIT port : [Attachment 32047] Add YARR support for generic ARM platforms (v2)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at inf.u-szeged.hu>'s request for review:
Bug 24986: ARM JIT port
https://bugs.webkit.org/show_bug.cgi?id=24986

Attachment 32047: Add YARR support for generic ARM platforms (v2)
https://bugs.webkit.org/attachment.cgi?id=32047&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
This all looks really great, couple of small comments.

In the MacroAssembler, we normally try to only use UNUSED_PARAM if a parameter
is only not use on some (ifdef'ed) code paths, so we'd normally write:

void mulDouble(Address src, FPRegisterID dest)
{
    UNUSED_PARAM(src);
    UNUSED_PARAM(dest);
    ASSERT_NOT_REACHED();
}

as:

void mulDouble(Address, FPRegisterID)
{
    ASSERT_NOT_REACHED();
}

But if you prefer to leave this unchanged, this would be okay.

In Platform.h:

 546 #elif PLATFORM(ARM)
 547	 /* Under development, temporarily disabled. */
 548	 #define ENABLE_JIT 0
 549	 #define ENABLE_JIT_OPTIMIZE_NATIVE_CALL 0

This should really be added as a part of the JIT patch, let's revert this for
now (it really doesn't make a lot of sense without the rest of the JIT changes
:o) ).	Also...

 596  || (!defined(ENABLE_YARR_JIT) && PLATFORM(ARM) && 0) \

We will only want to enable YARR & the ARM JIT on operating systems it has been
tested on.  What OS are you testing on?  Linux?  GTK or KDE?  Or other?  Also,
if Linux, this may also be the wrong place to be enabling them.  On Linux I
believe the platform specific settings are configured in configure.ac (GTK?)
and JavaScriptCore/JavaScriptCore.pri (KDE?) – and I believe this is where YARR
is enabled on x86-Linux.  (Personally, I find the fact that we configure these
things in different places on different OSes a little odd, but given that we
do, it certainly seems that it would be more correct to keep the way we do
things on ARM-Linux in line with the way we work on x86-Linux - again, assuming
that it is Linux you're targeting).

The rest of the patch all looks great.	r-ing while the Platform.h questions
are answered, but otherwise this all looks good to land.