Bug 34424 - Soft modulo routine for ARM_TRADITIONAL
Summary: Soft modulo routine for ARM_TRADITIONAL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-01 06:09 PST by Tamas Szirbucz
Modified: 2010-02-09 00:54 PST (History)
5 users (show)

See Also:


Attachments
Use an easily appendable structure for trampolines instead of pointer parameters. (8.96 KB, patch)
2010-02-01 06:13 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
Add a soft modulo operation to ARM JIT using a trampoline function. (11.90 KB, patch)
2010-02-01 06:14 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
SunSpider results on ARMv7 (3.56 KB, text/plain)
2010-02-01 06:21 PST, Tamas Szirbucz
no flags Details
Well formated patch for trampoline structures. (Use it first) (9.18 KB, patch)
2010-02-03 09:42 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
Well formated patch for soft modulo operation on ARM_TRADITIONAL. (Use it second) (10.54 KB, patch)
2010-02-03 09:46 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
Trampoline structure (use it first) (9.18 KB, patch)
2010-02-04 00:53 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
Soft modulo routine for ARM_TRADITIONAL (use it second) (10.53 KB, patch)
2010-02-04 00:56 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
Add a soft modulo operation to ARM JIT using a trampoline function (use it second). (10.57 KB, patch)
2010-02-04 07:40 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
macro proposal (890 bytes, patch)
2010-02-04 23:57 PST, Gabor Loki
no flags Details | Formatted Diff | Diff
ARM soft modulo with proposed changes (11.05 KB, patch)
2010-02-05 02:59 PST, Tamas Szirbucz
no flags Details | Formatted Diff | Diff
ARM soft modulo with proposed changes (11.13 KB, patch)
2010-02-05 05:06 PST, Tamas Szirbucz
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tamas Szirbucz 2010-02-01 06:09:49 PST
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.
Comment 1 Tamas Szirbucz 2010-02-01 06:13:16 PST
Created attachment 47834 [details]
Use an easily appendable structure for trampolines instead of pointer parameters.
Comment 2 Tamas Szirbucz 2010-02-01 06:14:09 PST
Created attachment 47835 [details]
Add a soft modulo operation to ARM JIT using a trampoline function.
Comment 3 Tamas Szirbucz 2010-02-01 06:21:15 PST
Created attachment 47836 [details]
SunSpider results on ARMv7

It looks like slowdowns are caused by change of the binary.
Comment 4 Tamas Szirbucz 2010-02-03 09:42:35 PST
Created attachment 48045 [details]
Well formated patch for trampoline structures. (Use it first)

id=47834 attachment's well formated version.
Comment 5 Tamas Szirbucz 2010-02-03 09:46:08 PST
Created attachment 48046 [details]
Well formated patch for soft modulo operation on ARM_TRADITIONAL. (Use it second)

id=47835 attachment's well formated version.
Comment 6 WebKit Review Bot 2010-02-03 09:49:27 PST
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.
Comment 7 Tamas Szirbucz 2010-02-04 00:53:35 PST
Created attachment 48112 [details]
Trampoline structure (use it first)

Sorry for the many attachments! I used wrong the check-webkit-style at last time.
Comment 8 Tamas Szirbucz 2010-02-04 00:56:46 PST
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.
Comment 9 Tamas Szirbucz 2010-02-04 07:40:30 PST
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.
Comment 10 Gavin Barraclough 2010-02-04 11:47:18 PST
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+.
Comment 11 Gabor Loki 2010-02-04 13:45:37 PST
> #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. :(
Comment 12 Tamas Szirbucz 2010-02-04 14:08:25 PST
(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.
Comment 13 Tamas Szirbucz 2010-02-04 14:10:59 PST
(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.
Comment 14 Gabor Loki 2010-02-04 14:17:01 PST
Comment on attachment 48112 [details]
Trampoline structure (use it first)

Landed (http://trac.webkit.org/changeset/54362)
Comment 15 Gabor Loki 2010-02-04 23:57:45 PST
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).
Comment 16 Tamas Szirbucz 2010-02-05 02:59:17 PST
Created attachment 48218 [details]
ARM soft modulo with proposed changes
Comment 17 Tamas Szirbucz 2010-02-05 05:06:02 PST
Created attachment 48222 [details]
 ARM soft modulo with proposed changes
Comment 18 Eric Seidel (no email) 2010-02-08 15:09:43 PST
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.
Comment 19 Gabor Loki 2010-02-09 00:54:03 PST
Attachment 48222 [details] is landed in r54534.
All patches are landed, so I close this bug as fixed.