Changeset 166716 in webkit


Ignore:
Timestamp:
Apr 3, 2014 7:37:32 AM (10 years ago)
Author:
mark.lam@apple.com
Message:

ARMv7 compare32() should not use TST to do CMP's job.
<https://webkit.org/b/131146>

Reviewed by Geoffrey Garen.

The ARMv7 implementation of "compare32(RegisterID left, TrustedImm32 right)"
was using "tst reg, reg" to implement "cmp reg, #0". Unfortunately, the tst
instruction doesn't set the Overflow (V) flag and this results in random
results depending on whether there was a preceeding instruction that did set
the Overflow (V) flag. This issue was causing emscripten-cube2hash to run
with a lot of OSR exits where not expected as well as producing wrong results.

The fix is to use "cmp reg, #0" to do the job properly.

  • assembler/MacroAssemblerARMv7.h:

(JSC::MacroAssemblerARMv7::compare32):

Location:
trunk/Source/JavaScriptCore
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r166678 r166716  
     12014-04-02  Mark Lam  <mark.lam@apple.com>
     2
     3        ARMv7 compare32() should not use TST to do CMP's job.
     4        <https://webkit.org/b/131146>
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        The ARMv7 implementation of "compare32(RegisterID left, TrustedImm32 right)"
     9        was using "tst reg, reg" to implement "cmp reg, #0".  Unfortunately, the tst
     10        instruction doesn't set the Overflow (V) flag and this results in random
     11        results depending on whether there was a preceeding instruction that did set
     12        the Overflow (V) flag.  This issue was causing emscripten-cube2hash to run
     13        with a lot of OSR exits where not expected as well as producing wrong results.
     14
     15        The fix is to use "cmp reg, #0" to do the job properly.
     16
     17        * assembler/MacroAssemblerARMv7.h:
     18        (JSC::MacroAssemblerARMv7::compare32):
     19
    1202014-04-02  Mark Hahnenberg  <mhahnenberg@apple.com>
    221
  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

    r164764 r166716  
    12881288    {
    12891289        int32_t imm = right.m_value;
    1290         if (!imm)
    1291             m_assembler.tst(left, left);
     1290        ARMThumbImmediate armImm = ARMThumbImmediate::makeEncodedImm(imm);
     1291        if (armImm.isValid())
     1292            m_assembler.cmp(left, armImm);
     1293        else if ((armImm = ARMThumbImmediate::makeEncodedImm(-imm)).isValid())
     1294            m_assembler.cmn(left, armImm);
    12921295        else {
    1293             ARMThumbImmediate armImm = ARMThumbImmediate::makeEncodedImm(imm);
    1294             if (armImm.isValid())
    1295                 m_assembler.cmp(left, armImm);
    1296             else if ((armImm = ARMThumbImmediate::makeEncodedImm(-imm)).isValid())
    1297                 m_assembler.cmn(left, armImm);
    1298             else {
    1299                 move(TrustedImm32(imm), dataTempRegister);
    1300                 m_assembler.cmp(left, dataTempRegister);
    1301             }
     1296            move(TrustedImm32(imm), dataTempRegister);
     1297            m_assembler.cmp(left, dataTempRegister);
    13021298        }
    13031299    }
Note: See TracChangeset for help on using the changeset viewer.