[webkit-reviews] review denied: [Bug 115138] [QNX][ARM] Use hardware integer division where available : [Attachment 199537] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Cosmin Truta
<ctruta at gmail.com>'s request for review:
Bug 115138: [QNX][ARM] Use hardware integer division where available
https://bugs.webkit.org/show_bug.cgi?id=115138

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=199537&action=review


Nope.

> Source/JavaScriptCore/dfg/DFGCommon.h:109
>  #if CPU(APPLE_ARMV7S)
>      return true;
> +#elif CPU(ARM_THUMB2) && OS(QNX)
> +    return MacroAssembler::supportsIntegerDiv();

This is terribly misleading.

What you should do is:
    -MacroAssembler::supportsIntegerDiv() is a _compile time_ constant for
everything but QNX
    -It is true for ARMv7s, false everywhere else.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2256
>  #elif CPU(APPLE_ARMV7S)
> -	       compileIntegerArithDivForARMv7s(node);
> +	       compileIntegerArithDivForARM(node);
> +#elif CPU(ARM_THUMB2) && OS(QNX)
> +	       if (MacroAssembler::supportsIntegerDiv())
> +		   compileIntegerArithDivForARM(node);

That makes no sense. What if you  are ArithDiv and
MacroAssembler::supportsIntegerDiv() evaluate to false, you just silently
ignore the operation?