585272 - JM: convert result of division to integer, if possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/10.04 (lucid) Firefox/3.6.8
Build Identifier: 

Currently, x/y always results in a double. 
I will attach a patch to fix this.

Reproducible: Always

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch v1

V8 and JSC do different things. V8 tries integer division and checks the remainder. If the remainder is non-zero, it does double division. JSC always does double-division and checks if the result fits in a double.

Initially I tried the same approach as V8, but performance did not improve that much, even for integer results. Also, it gets quite complex, we have to generate the following paths: 1) Inline int division 2) Double division 3) OOL stub call 4) Double division when remainder is non-zero.

So the attached patch does it like JSC, it checks if the result fits in an integer. I also added an optimization to ignore this check for 1/x, as it's unlikely to fit in an int.

This is a win on spectral-norm (28 ms. -> 20 ms. on my old P4) and a tiny win on some other tests.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 463770 [details] [diff] [review]
Patch v1

The patch looks good. 64-bit can be a bit smarter with the loads, but that can be patched later. Based on knowledge of how often the integer path is taken, we might also considering always ending the function with the payload borne in a register.

Comment 3

[Sean Stangl [:sstangl]]

Note that the line should be "masm.storeTypeTag(ImmType(JSVAL_TYPE_INT32), frame.addressOf(lhs));".

Comment 4

[Andrew Drake [:adrake]]

Passes trace and jstests for me.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Addresses comment 3, add a fixme for 64-bit, also ignore -1/x as pointed out by sstangl on IRC.

Comment 6

[Jan de Mooij [:jandem]]

SS results on my prehistoric machine:

TEST                   COMPARISON            FROM                 TO         

=============================================================================

** TOTAL **:           1.020x as fast    950.4ms +/- 0.5%   931.8ms +/- 0.4% 

=============================================================================

  3d:                  *1.012x as slow*  125.3ms +/- 0.7%   126.8ms +/- 0.9% 
    cube:              ??                 41.5ms +/- 0.9%    42.0ms +/- 0.7% 
    morph:             ??                 36.1ms +/- 1.7%    36.3ms +/- 1.7% 
    raytrace:          *1.021x as slow*   47.6ms +/- 0.9%    48.6ms +/- 1.6% 

  access:              *1.022x as slow*  100.5ms +/- 0.8%   102.7ms +/- 1.9% 
    binary-trees:      ??                 32.5ms +/- 1.3%    32.9ms +/- 1.8% 
    fannkuch:          ??                 25.1ms +/- 1.3%    25.6ms +/- 3.9% 
    nbody:             *1.032x as slow*   31.1ms +/- 1.0%    32.1ms +/- 1.7% 
    nsieve:            ??                 11.8ms +/- 2.3%    12.0ms +/- 6.3% 

  bitops:              -                  44.1ms +/- 2.5%    43.3ms +/- 2.4% 
    3bit-bits-in-byte: -                   9.8ms +/- 3.8%     9.3ms +/- 5.2% 
    bits-in-byte:      -                  17.2ms +/- 3.0%    16.9ms +/- 2.6% 
    bitwise-and:       -                   6.9ms +/- 3.7%     6.8ms +/- 2.5% 
    nsieve-bits:       ??                 10.3ms +/- 3.3%    10.3ms +/- 3.0% 

  controlflow:         -                  19.9ms +/- 3.5%    18.9ms +/- 5.8% 
    recursive:         -                  19.9ms +/- 3.5%    18.9ms +/- 5.8% 

  crypto:              1.053x as fast     59.0ms +/- 1.6%    56.0ms +/- 1.9% 
    aes:               1.031x as fast     26.8ms +/- 1.4%    25.9ms +/- 1.7% 
    md5:               1.064x as fast     18.3ms +/- 3.0%    17.2ms +/- 3.8% 
    sha1:              1.082x as fast     13.9ms +/- 2.9%    12.8ms +/- 3.4% 

  date:                1.015x as fast    146.6ms +/- 1.3%   144.5ms +/- 0.7% 
    format-tofte:      *1.013x as slow*   62.0ms +/- 0.8%    62.8ms +/- 1.0% 
    format-xparb:      1.035x as fast     84.7ms +/- 2.4%    81.8ms +/- 0.9% 

  math:                1.105x as fast     95.5ms +/- 0.8%    86.4ms +/- 0.6% 
    cordic:            -                  23.6ms +/- 1.9%    23.1ms +/- 1.3% 
    partial-sums:      1.018x as fast     43.4ms +/- 1.5%    42.6ms +/- 0.8% 
    spectral-norm:     1.38x as fast      28.6ms +/- 1.7%    20.7ms +/- 1.5% 

  regexp:              1.046x as fast     35.4ms +/- 1.9%    33.8ms +/- 1.4% 
    dna:               1.046x as fast     35.4ms +/- 1.9%    33.8ms +/- 1.4% 

  string:              1.015x as fast    324.3ms +/- 0.7%   319.4ms +/- 0.4%
    base64:            ??                 17.2ms +/- 2.6%    17.3ms +/- 1.7%
    fasta:             1.030x as fast     45.0ms +/- 1.7%    43.8ms +/- 1.0%
    tagcloud:          1.020x as fast    114.3ms +/- 1.0%   112.1ms +/- 0.4%
    unpack-code:       -                 110.7ms +/- 1.2%   109.6ms +/- 0.6% 
    validate-input:    -                  37.0ms +/- 0.8%    36.7ms +/- 1.2%

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch v3

Add instruction spew for jne and jp, by forwarding to jCC.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 463822 [details] [diff] [review]
Patch v3

Awesome, thanks again Jan! Will push momentarily, can fix the nits myself:

>+    MaybeJump done;
>+    
>+    /* Try to convert result to integer. Skip this for 1/x or -1/x, as 
>+     * the result is unlikely to fit in an int. 
>+     * :FIXME: loads on 64-bit can be smarter. */

House multiline comment style for is either // or 

/*
 * Try to...
 */

>+        done.setJump(masm.jump());

This can just be: done = masm.jump();

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/deb91b6b1eca