Bug 49432 – Support JIT_OPTIMIZE_MOD on Thumb-2

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gabor Loki]

The current implementation of integer-modulo is not effective on ARM with Thumb-2. The ARM_TRADITIONAL has had JIT_OPTIMIZE_MODJIT_OPTIMIZE_MOD which dramatically increases the performance of integer-modulo. The same algorithm can be used on Thumb-2 JIT.

[Gabor Loki]

Created attachment 73718 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

SunSpider: 1.043x as fast

An artificial modulo test: 6.5x as fast

[Gabor Loki]

Created attachment 73721 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

remove an unused condition.

[Gavin Barraclough]

Comment on attachment 73721 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

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

Hey Loki,

Looks like a great patch, a couple of small issues.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:1719
> +#if CPU(ARM_THUMB2)
> +    RegisterID regS0 = ARMRegisters::r6;
> +    RegisterID regS1 = ARMRegisters::r7;
> +#endif

I'm not sure about defining these register names up here.  Currently all registers used by the JIT are effectively documented in JSInterfaceJIT.h, if someone wants to use a register they have one place to go look to see if it is used by the JIT, needs preserving in the entry trampolines, etc.  My concern is that a developer won't know to come look here to discover these registers are used.  I think it would be better to add definitions for regT5 & regT6 in JSInterfaceJIT.h, for ARM & ARMv7.

> JavaScriptCore/assembler/MacroAssemblerARMv7.h:226
> +    void clz(RegisterID src, RegisterID dest)
> +    {
> +        m_assembler.clz(dest, src);
> +    }

I'm not sure about this method.  The first question is should this be in the MacroAssembler interface at all? - do we believe it can be usefully abstracted across a range of platforms? – x86's BSR has somewhat different semantics, so I'm not sure we'll want to generally use this call in a cross platform way - but maybe we'd abstract like this (I find BSR a little weird to use), so I'm inclined to lean towards yes, keep this here (the alternative being to just call to m_assembler.clz directly).  But if we keep this, the name should change.  At minimum the operation size should be encoded in the name - clz32() (the name clz is correct at the assembler layer, this is just for the MacroAssemlber layer).  But coding style ask us to steer clear on acronyms & abbreviations (unless they're particularly standard and conventional), and I think we should probably change clz32 to something more explicit - like countLeadingZeros32().  What do you think?

r+ with these changes, r-ing this patch.

cheers,
G.

[Gabor Loki]

Created attachment 74486 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

List of changes:
* The clz functions are renamed to countLeadingZeros32 in MacroAssembler interface.
* I was able to remove the two extra temporary registers using one extra space from stack.
* A possible bug was fixed related to ARMv4.
* The performance numbers are not changed.

[Gavin Barraclough]

Comment on attachment 74486 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

Looks awesome Loki.  Just spotted that:
> #error "JIT_OPTIMIZE_MOD not yet supported on this platform."
should now be
> #error "JIT_USE_SOFT_MODULO not yet supported on this platform."
but that's my bad from an earlier change, and won't hold your patch up further for this – will fix later.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74486 [details]:

http/tests/appcache/credential-url.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74486 [details]
Support JIT_OPTIMIZE_MOD on Thumb-2

Clearing flags on attachment: 74486

Committed r72481: <http://trac.webkit.org/changeset/72481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74486 [details]:

fast/profiler/throw-exception-from-eval.html
fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.