[webkit-reviews] review denied: [Bug 44329] SH4 JIT SUPPORT : [Attachment 88415] JIT remaining part for SH4 platforms.

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied thouraya <thouraya.andolsi at st.com>'s
request for review:
Bug 44329: SH4 JIT SUPPORT
https://bugs.webkit.org/show_bug.cgi?id=44329

Attachment 88415: JIT remaining part for SH4 platforms.
https://bugs.webkit.org/attachment.cgi?id=88415&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88415&action=review

r- due to the END_UNINTERRUPTED_SEQUENCE stuff

> Source/JavaScriptCore/assembler/SH4Assembler.h:36
> +#include <stdio.h>

this seems unnecessary

> Source/JavaScriptCore/jit/JIT.h:712
> +#define END_UNINTERRUPTED_SEQUENCES(name, extraI, extraC)
endUninterruptedSequence(name ## InstructionSpace + extraI, name ##
ConstantSpace + extraC)

macros like this should always be wrapped in a do {} while (false) construct to
reduce the chance of incorrect use.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:485
> +#if CPU(SH4)
> +    if ((dst >= 0) && (dst <= 7))
> +	   END_UNINTERRUPTED_SEQUENCE(sequenceGetByIdSlowCase);
> +    else if ((dst > 15) || (dst < -16))
> +	   END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 2);
> +    else
> +	   END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 0);
> +#else

Rather than having this ifdef here you should have this logic in the platform
specific handler for uninterrupted sequence.  If absolutely necessary you can
just make END_UNINTERRUPTED_SEQUENCE takes a dst parameter.