Bug 115722 – [sh4] Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

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

[Julien Brianceau]

Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT.

[Julien Brianceau]

Created attachment 200892 [details]
Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/MacroAssemblerSH4.h', u'Source/JavaScriptCore/assembler/SH4Assembler.h']" exit_code: 1
Source/JavaScriptCore/assembler/SH4Assembler.h:120:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 3 files


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

[Julien Brianceau]

Comment on attachment 200892 [details]
Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

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

>> Source/JavaScriptCore/assembler/SH4Assembler.h:120
>> +    MOVW_READ_RMINC_OPCODE = 0x6005,
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

All other 100+ entries in this enum are in capital letters, so I think I'll leave it as it is.

[Julien Brianceau]

I didn't see any regression when testing this patch with:
- Tools/Scripts/run-javascriptcore-tests
- Tools/Scripts/run-fast-jsc


Very little speed improvement according to SunSpider with this patch:
Rev 149664 without patch:
============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 13461.8ms +/- 0.3%
--------------------------------------------

  3d:                   2751.5ms +/- 0.2%
    cube:                838.5ms +/- 0.7%
    morph:               500.5ms +/- 0.2%
    raytrace:           1412.5ms +/- 0.4%

  access:               1948.0ms +/- 0.1%
    binary-trees:        136.5ms +/- 0.7%
    fannkuch:           1176.8ms +/- 0.4%
    nbody:               454.8ms +/- 1.2%
    nsieve:              180.0ms +/- 1.8%

  bitops:               1051.8ms +/- 0.5%
    3bit-bits-in-byte:    94.8ms +/- 0.8%
    bits-in-byte:        122.0ms +/- 0.0%
    bitwise-and:         202.8ms +/- 2.7%
    nsieve-bits:         632.3ms +/- 0.8%

  controlflow:           105.5ms +/- 4.5%
    recursive:           105.5ms +/- 4.5%

  crypto:               1236.0ms +/- 0.2%
    aes:                 725.0ms +/- 0.8%
    md5:                 328.3ms +/- 1.0%
    sha1:                182.8ms +/- 0.4%

  date:                 1983.0ms +/- 0.7%
    format-tofte:        927.3ms +/- 0.6%
    format-xparb:       1055.8ms +/- 1.8%

  math:                  988.5ms +/- 2.1%
    cordic:              289.8ms +/- 2.0%
    partial-sums:        395.5ms +/- 1.5%
    spectral-norm:       303.3ms +/- 6.0%

  regexp:                563.3ms +/- 0.1%
    dna:                 563.3ms +/- 0.1%

  string:               2834.3ms +/- 0.8%
    base64:              224.5ms +/- 3.3%
    fasta:               459.0ms +/- 9.8%
    tagcloud:            568.3ms +/- 0.9%
    unpack-code:        1242.5ms +/- 4.2%
    validate-input:      340.0ms +/- 2.5%




Rev 149664 with patch:
============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 13311.0ms +/- 0.5%
--------------------------------------------

  3d:                   2599.8ms +/- 0.4%
    cube:                834.5ms +/- 0.4%
    morph:               446.3ms +/- 0.9%
    raytrace:           1319.0ms +/- 1.0%

  access:               1957.3ms +/- 0.5%
    binary-trees:        137.3ms +/- 3.7%
    fannkuch:           1180.0ms +/- 1.1%
    nbody:               456.5ms +/- 0.9%
    nsieve:              183.5ms +/- 0.5%

  bitops:               1059.3ms +/- 0.4%
    3bit-bits-in-byte:    94.5ms +/- 1.7%
    bits-in-byte:        121.5ms +/- 0.8%
    bitwise-and:         202.0ms +/- 0.0%
    nsieve-bits:         641.3ms +/- 0.8%

  controlflow:           103.3ms +/- 0.8%
    recursive:           103.3ms +/- 0.8%

  crypto:               1236.3ms +/- 0.9%
    aes:                 739.8ms +/- 1.1%
    md5:                 313.5ms +/- 0.5%
    sha1:                183.0ms +/- 3.0%

  date:                 2020.0ms +/- 1.5%
    format-tofte:        949.0ms +/- 1.6%
    format-xparb:       1071.0ms +/- 3.4%

  math:                  987.0ms +/- 2.7%
    cordic:              293.0ms +/- 2.3%
    partial-sums:        410.8ms +/- 7.9%
    spectral-norm:       283.3ms +/- 0.3%

  regexp:                532.5ms +/- 1.2%
    dna:                 532.5ms +/- 1.2%

  string:               2815.8ms +/- 1.1%
    base64:              213.8ms +/- 2.0%
    fasta:               452.5ms +/- 9.2%
    tagcloud:            588.8ms +/- 3.3%
    unpack-code:        1189.3ms +/- 4.8%
    validate-input:      371.5ms +/- 2.9%

[Oliver Hunt]

Comment on attachment 200892 [details]
Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

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

I had some war moments while reading this, so i'll hold off on r+ pending replies

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1043
> -        m_assembler.loadConstant(address.offset, scr);
> +        m_assembler.loadConstant(address.offset + 8, scr);

wat?

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1057
> +        add32(TrustedImm32(address.offset + 8), scr);

again wat?

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1170
> -        m_assembler.ensureSpace(m_assembler.maxInstructionSize + 68, sizeof(uint32_t));
> +        m_assembler.ensureSpace(m_assembler.maxInstructionSize + 58, sizeof(uint32_t));

This magic constant scares me, what is it?

>>> Source/JavaScriptCore/assembler/SH4Assembler.h:120
>>> +    MOVW_READ_RMINC_OPCODE = 0x6005,
>> 
>> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> 
> All other 100+ entries in this enum are in capital letters, so I think I'll leave it as it is.

I think we just need to modify the style bot to ignore these

[Julien Brianceau]

Comment on attachment 200892 [details]
Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

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

>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1043
>> +        m_assembler.loadConstant(address.offset + 8, scr);
> 
> wat?

Before patch we had something like:
1. load effective address in scr
2. store first part of the double at address scr
3. add 4 to scr
4. store second part of the double at address scr

With patch, I do this as it is in LLINT:
1. load effective address + 8 in scr
2. store the second part of the double at address scr with predecrement of scr (-4)
3. store the first part of the double at address scr with predecrement of scr (-4)

Result is the same but we save one opcode.

>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1057
>> +        add32(TrustedImm32(address.offset + 8), scr);
> 
> again wat?

Same idea (and I removed the if, because this case is already checked in add32() function).

>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1170
>> +        m_assembler.ensureSpace(m_assembler.maxInstructionSize + 58, sizeof(uint32_t));
> 
> This magic constant scares me, what is it?

It scares me too :/
From what I understand, it ensures that we'll have enough contiguous space in the instruction buffer (to be able to do relative short jumps for instance).
I reduced the constant size from 10 because I removed 10 bytes of instructions, but I'm not very proud of this...

>>>> Source/JavaScriptCore/assembler/SH4Assembler.h:120
>>>> +    MOVW_READ_RMINC_OPCODE = 0x6005,
>>> 
>>> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
>> 
>> All other 100+ entries in this enum are in capital letters, so I think I'll leave it as it is.
> 
> I think we just need to modify the style bot to ignore these

Ok

[WebKit Commit Bot]

Comment on attachment 200892 [details]
Take advantage of pre-decrement and post-increment opcodes for sh4 base JIT

Clearing flags on attachment: 200892

Committed r149676: <http://trac.webkit.org/changeset/149676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.