Bug 115138 – [ARM] Expand the use of integer division

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Cosmin Truta]

Currently, the ARM SDIV and UDIV instructions are used only on ARM v7s. We also enable them on QNX.

However, on QNX, the same JavaScriptCore build runs on various devices that may or may not have these instructions, so we need to check the availability of hardware integer division at startup time.

A patch will follow.

[Cosmin Truta]

Created attachment 199536 [details]
Patch

Side note: this can be easily extended to other systems (e.g. Linux on ARM) that need to know about hardware idiv availability at run time, by adding the appropriate checks to isIntegerDivSupported().

[Cosmin Truta]

Created attachment 199537 [details]
Patch

Fixed style.

[Benjamin Poulain]

Comment on attachment 199537 [details]
Patch

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?

[Cosmin Truta]

Created attachment 199607 [details]
Patch

(In reply to comment #3)
> What you should do is:
>     -MacroAssembler::supportsIntegerDiv() is a _compile time_ constant for everything but QNX
>     [...]

Done. This allowed me to simplify preprocessor conditionals in other places, and rely on the fact that this gets resolved at compile time whenever possible.

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

From what I understood so far, the call site is responsible not to branch here if IDIV isn't available, hence the RELEASE_ASSERT_NOT_REACHED further below. And you're right, that if-statement shouldn't be there. I changed it into an assertion.

[Cosmin Truta]

Created attachment 199614 [details]
Patch

I initially made DFG::isARMWithIntegerDiv() in order to use it as it's the case with DFG::isX86(), i.e. when it's not protected by #if CPU(...); but then I noticed I had used it inside in ARM-specific code also, interchangeably (and inconsistently) with MacroAssembler::supportsIntegerDiv(). I am now modifying the previous patch: I use MacroAssembler::supportsIntegerDiv() consistently inside ARM-specific code, and DFG::isARMWithIntegerDiv() only in the rare cases outside.

I also simplified isIntegerDivSupported().

[Benjamin Poulain]

Comment on attachment 199614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199614&action=review

Better but there is still some confusions with your flags.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:49
> +namespace JSC {
> +
> +static bool isIntegerDivSupported()
> +{
> +#if CPU(APPLE_ARMV7S)
> +    return true;
> +#elif OS(QNX) && defined(ARM_CPU_FLAG_IDIV)
> +    return !!(SYSPAGE_ENTRY(cpuinfo)->flags & ARM_CPU_FLAG_IDIV);
> +#else
> +    return false;
> +#endif
> +}
> +
> +const bool MacroAssemblerARMv7::s_isIntegerDivSupported = isIntegerDivSupported();

This whole code could be in if OS(QNX).
You don't need ARMV7S and the fallback.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1921
> +
> +    static const bool s_isIntegerDivSupported;

This should be in #if OS(QNX)

[Cosmin Truta]

Created attachment 199673 [details]
Patch

(In reply to comment #6)
> This whole code could be in if OS(QNX).
> You don't need ARMV7S and the fallback.

Done.

[Cosmin Truta]

I am changing the summary line to reflect that improvements are no longer limited to hardware IDIV, or to QNX.
Specifically, I implemented a speedup for the software-emulated modulo operation, by replacing fmod() with div().

[Cosmin Truta]

Created attachment 199890 [details]
Patch

Although I only tested this on ARM, I believe that using div() instead of fmod() will benefit virtually all the other platforms also, if the necessary changes are made inside compileSoftModulo().

[Cosmin Truta]

Created attachment 199891 [details]
Patch

Fixed a line in ChangeLog.

[Benjamin Poulain]

Comment on attachment 199891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199891&action=review

Looks reasonable.

Some comments bellow. I would prefer if Filip could double check before you land.

> Source/JavaScriptCore/ChangeLog:38
> +        (JSC::DFG::SpeculativeJIT::compileSoftModuloForX86): Added.
> +        (JSC::DFG::SpeculativeJIT::compileSoftModuloForARM): Added.
> +        (JSC::DFG::SpeculativeJIT::compileSoftModulo): Split off the X86 and ARM codegen.

Why not just name them all compileSoftModulo?
That would make the call site simpler.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1607
> +    div_t quotientAndRemainder = div(a, b);
> +    return quotientAndRemainder.rem;

a % b?

[Cosmin Truta]

Created attachment 200069 [details]
Patch

Addressing reviewer's comments.

[Filip Pizlo]

Comment on attachment 200069 [details]
Patch

I like this.  R=me too.

[Benjamin Poulain]

Comment on attachment 200069 [details]
Patch

(In reply to comment #13)
> (From update of attachment 200069 [details])
> I like this.  R=me too.

Thanks for checking the patch. Let's cq.

[WebKit Commit Bot]

Comment on attachment 200069 [details]
Patch

Clearing flags on attachment: 200069

Committed r149349: <http://trac.webkit.org/changeset/149349>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

A speculative build fix landed for Qt ARM and Mips in http://trac.webkit.org/changeset/149354.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 115443

[Csaba Osztrogonác]

Comment on attachment 199891 [details]
Patch

Cleared Benjamin Poulain's review+ from obsolete attachment 199891 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Csaba Osztrogonác]

Is there any reason why is it closed as RESOLVED/WONTFIX?