Bug 34424 – Soft modulo routine for ARM_TRADITIONAL

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

[Tamas Szirbucz]

On ARM there is not any modulo assembly instruction, but in case of integers it can be implemented with a simply routine.
I will attach patches for that.

[Tamas Szirbucz]

Created attachment 47834 [details]
Use an easily appendable structure for trampolines instead of pointer parameters.

[Tamas Szirbucz]

Created attachment 47835 [details]
Add a soft modulo operation to ARM JIT using a trampoline function.

[Tamas Szirbucz]

Created attachment 47836 [details]
SunSpider results on ARMv7

It looks like slowdowns are caused by change of the binary.

[Tamas Szirbucz]

Created attachment 48045 [details]
Well formated patch for trampoline structures. (Use it first)

id=47834 attachment's well formated version.

[Tamas Szirbucz]

Created attachment 48046 [details]
Well formated patch for soft modulo operation on ARM_TRADITIONAL. (Use it second)

id=47835 attachment's well formated version.

[WebKit Review Bot]

Attachment 48045 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/jit/JITStubs.h:78:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tamas Szirbucz]

Created attachment 48112 [details]
Trampoline structure (use it first)

Sorry for the many attachments! I used wrong the check-webkit-style at last time.

[Tamas Szirbucz]

Created attachment 48113 [details]
Soft modulo routine for ARM_TRADITIONAL (use it second)

Sorry for the many attachments! I used wrong the check-webkit-style at last time.

[Tamas Szirbucz]

Created attachment 48140 [details]
 Add a soft modulo operation to ARM JIT using a trampoline function (use it second).

The previous attachment caused compile errors on X86.

[Gavin Barraclough]

Comment on attachment 48140 [details]
 Add a soft modulo operation to ARM JIT using a trampoline function (use it second).

Okay, this basically all looks great.  A couple of small things:

(1) One of the implementations of 'emitSlow_op_mod' contains an 'ASSERT_NOT_REACHED();', one doesn't (before and after your patch).  I think this should be there in both cases, unless I'm missing something please add this to the case without the assert.
(2) For configurable JIT optimizations we've been using switches of the format "ENABLE_JIT_OPTIMIZE_???", and I think it would make sense to move the switch to bring it in line with these.  (part of the reason is that it may be nice at some point to move these out of Platform so that changing these switches only means recompiling the JIT rather than all of WebCore).  I'd suggest "ENABLE_JIT_OPTIMIZE_MOD", alongside the other JIT_OPTIMIZE switches, and I'd suggest defining it as something like:

#ifndef ENABLE_JIT_OPTIMIZE_MOD
#define ENABLE_JIT_OPTIMIZE_MOD (CPU(ARM_TRADITIONAL) && WTF_ARM_ARCH_AT_LEAST(5))
#endif

With these changes, r+.

[Gabor Loki]

> #ifndef ENABLE_JIT_OPTIMIZE_MOD
> #define ENABLE_JIT_OPTIMIZE_MOD (CPU(ARM_TRADITIONAL) &&
> WTF_ARM_ARCH_AT_LEAST(5))
> #endif

This will not work for non-ARM architectures, because there is no way to break the evaluation of logical expression in a #define. In x86 the #if ENABLE(JIT_OPTIMIZE_MOD) will turn into something similar:
#if defined ENABLE_JIT_OPTIMIZE_MOD && CPU(ARM_TRADITIONAL) && WTF_ARM_ARCH_AT_LEAST(5), where WTF_ARM_ARCH_AT_LEAST is not defined. :(

[Tamas Szirbucz]

(In reply to comment #11)
> > #ifndef ENABLE_JIT_OPTIMIZE_MOD
> > #define ENABLE_JIT_OPTIMIZE_MOD (CPU(ARM_TRADITIONAL) &&
> > WTF_ARM_ARCH_AT_LEAST(5))
> > #endif
> 
> This will not work for non-ARM architectures, because there is no way to break
> the evaluation of logical expression in a #define. In x86 the #if
> ENABLE(JIT_OPTIMIZE_MOD) will turn into something similar:
> #if defined ENABLE_JIT_OPTIMIZE_MOD && CPU(ARM_TRADITIONAL) &&
> WTF_ARM_ARCH_AT_LEAST(5), where WTF_ARM_ARCH_AT_LEAST is not defined. :(

I think so, if CPU(ARM_TRADITIONAL) is false, WTF_ARM_ARCH_AT_LEAST will be never evaluated in this expression.

[Tamas Szirbucz]

(In reply to comment #12)
> (In reply to comment #11)
> > > #ifndef ENABLE_JIT_OPTIMIZE_MOD
> > > #define ENABLE_JIT_OPTIMIZE_MOD (CPU(ARM_TRADITIONAL) &&
> > > WTF_ARM_ARCH_AT_LEAST(5))
> > > #endif
> > 
> > This will not work for non-ARM architectures, because there is no way to break
> > the evaluation of logical expression in a #define. In x86 the #if
> > ENABLE(JIT_OPTIMIZE_MOD) will turn into something similar:
> > #if defined ENABLE_JIT_OPTIMIZE_MOD && CPU(ARM_TRADITIONAL) &&
> > WTF_ARM_ARCH_AT_LEAST(5), where WTF_ARM_ARCH_AT_LEAST is not defined. :(
> 
> I think so, if CPU(ARM_TRADITIONAL) is false, WTF_ARM_ARCH_AT_LEAST will be
> never evaluated in this expression.

Ok, I misread that! Gabor, you are right.

[Gabor Loki]

Comment on attachment 48112 [details]
Trampoline structure (use it first)

Landed (http://trac.webkit.org/changeset/54362)

[Gabor Loki]

Created attachment 48207 [details]
macro proposal

Something like that ^^ can be used. Although it creates a new macro block (CPU optimizations), it looks better than the first approach. This change also implies to change armModFast functions name to something general (for example: softModulo).

[Tamas Szirbucz]

Created attachment 48218 [details]
ARM soft modulo with proposed changes

[Tamas Szirbucz]

Created attachment 48222 [details]
 ARM soft modulo with proposed changes

[Eric Seidel (no email)]

Comment on attachment 48140 [details]
 Add a soft modulo operation to ARM JIT using a trampoline function (use it second).

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

[Gabor Loki]

Attachment 48222 [details] is landed in r54534.
All patches are landed, so I close this bug as fixed.