Bug 115138 - [ARM] Expand the use of integer division
Summary: [ARM] Expand the use of integer division
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 108645 115443 115444
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-24 16:13 PDT by Cosmin Truta
Modified: 2014-02-14 01:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (21.00 KB, patch)
2013-04-24 16:56 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (20.96 KB, patch)
2013-04-24 16:59 PDT, Cosmin Truta
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Patch (21.00 KB, patch)
2013-04-24 19:13 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2013-04-24 20:46 PDT, Cosmin Truta
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2013-04-25 09:37 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (28.22 KB, patch)
2013-04-26 22:05 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (28.22 KB, patch)
2013-04-26 22:08 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (27.34 KB, patch)
2013-04-29 18:28 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2013-04-24 16:13:39 PDT
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.
Comment 1 Cosmin Truta 2013-04-24 16:56:44 PDT
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().
Comment 2 Cosmin Truta 2013-04-24 16:59:49 PDT
Created attachment 199537 [details]
Patch

Fixed style.
Comment 3 Benjamin Poulain 2013-04-24 17:11:16 PDT
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?
Comment 4 Cosmin Truta 2013-04-24 19:13:44 PDT
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.
Comment 5 Cosmin Truta 2013-04-24 20:46:59 PDT
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().
Comment 6 Benjamin Poulain 2013-04-24 22:23:41 PDT
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)
Comment 7 Cosmin Truta 2013-04-25 09:37:56 PDT
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.
Comment 8 Cosmin Truta 2013-04-26 22:00:24 PDT
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().
Comment 9 Cosmin Truta 2013-04-26 22:05:32 PDT
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().
Comment 10 Cosmin Truta 2013-04-26 22:08:18 PDT
Created attachment 199891 [details]
Patch

Fixed a line in ChangeLog.
Comment 11 Benjamin Poulain 2013-04-29 17:58:15 PDT
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?
Comment 12 Cosmin Truta 2013-04-29 18:28:20 PDT
Created attachment 200069 [details]
Patch

Addressing reviewer's comments.
Comment 13 Filip Pizlo 2013-04-29 18:33:39 PDT
Comment on attachment 200069 [details]
Patch

I like this.  R=me too.
Comment 14 Benjamin Poulain 2013-04-29 19:27:59 PDT
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.
Comment 15 WebKit Commit Bot 2013-04-29 19:54:19 PDT
Comment on attachment 200069 [details]
Patch

Clearing flags on attachment: 200069

Committed r149349: <http://trac.webkit.org/changeset/149349>
Comment 16 WebKit Commit Bot 2013-04-29 19:54:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Zoltan Arvai 2013-04-30 00:43:23 PDT
A speculative build fix landed for Qt ARM and Mips in http://trac.webkit.org/changeset/149354.
Comment 18 WebKit Commit Bot 2013-04-30 13:12:11 PDT
Re-opened since this is blocked by bug 115443
Comment 19 Csaba Osztrogonác 2014-02-13 04:04:36 PST
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.
Comment 20 Csaba Osztrogonác 2014-02-14 01:23:04 PST
Is there any reason why is it closed as RESOLVED/WONTFIX?