⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 252728 in webkit


Ignore:
Timestamp:
Nov 20, 2019, 10:02:50 PM (7 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] Extend MacroAssemblerARM64::load/store for datasize = 16
​https://bugs.webkit.org/show_bug.cgi?id=204442
<rdar://problem/57366761>

Reviewed by Mark Lam.

Our void load16(const void* address, RegisterID dest) and void store16(RegisterID src, const void* address) are not aware of
the condition that passed register can be memoryTempRegister, while MacroAssemblerARM64::{load,store} handles it correctly, e.g.
load invalidates cachedMemoryTempRegister if destination register is memoryTempRegister. As a result, when we are emitting
or16(TrustedImm32 imm, AbsoluteAddress address) with address where the address's value does not fit in imm, the generated code
is reusing memoryTempRegister incorrectly.

0xedf8d4fb4: mov x17, #0x7af0
0xedf8d4fb8: movk x17, #0xd5a, lsl #16
0xedf8d4fbc: movk x17, #0x1, lsl #32 Construct imm register on x17.
0xedf8d4fc0: ldrh w17, [x17] Load half word from x17 to w17 (we should invalidate x17 memoryTempRegister here).
0xedf8d4fc4: mov w16, #0x1b
0xedf8d4fc8: orr w16, w17, w16
0xedf8d4fcc: strh w16, [x17] x17 memoryTempRegister is reused while its content is invalid.

The problem is that load and store functions are not supporting datasize = 16 case. This patch extends MacroAssemblerARM64::{load,store}
to support 16 so that or16 implementation looks is similar to or32 etc.

  • assembler/MacroAssemblerARM64.h:

(JSC::MacroAssemblerARM64::load16):
(JSC::MacroAssemblerARM64::store16):
(JSC::MacroAssemblerARM64::load):
(JSC::MacroAssemblerARM64::store):

  • assembler/testmasm.cpp:

(JSC::testOrImmMem):

Location:
trunk/Source/JavaScriptCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r252725 r252728  
     12019-11-20  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Extend MacroAssemblerARM64::load/store for datasize = 16
     4        https://bugs.webkit.org/show_bug.cgi?id=204442
     5        <rdar://problem/57366761>
     6
     7        Reviewed by Mark Lam.
     8
     9        Our `void load16(const void* address, RegisterID dest)` and `void store16(RegisterID src, const void* address)` are not aware of
     10        the condition that passed register can be memoryTempRegister, while `MacroAssemblerARM64::{load,store}` handles it correctly, e.g.
     11        `load` invalidates `cachedMemoryTempRegister` if destination register is memoryTempRegister. As a result, when we are emitting
     12        `or16(TrustedImm32 imm, AbsoluteAddress address)` with address where the address's value does not fit in imm, the generated code
     13        is reusing memoryTempRegister incorrectly.
     14
     15            0xedf8d4fb4: mov    x17, #0x7af0
     16            0xedf8d4fb8: movk   x17, #0xd5a, lsl #16
     17            0xedf8d4fbc: movk   x17, #0x1, lsl #32        // Construct imm register on x17.
     18            0xedf8d4fc0: ldrh   w17, [x17]                // Load half word from x17 to w17 (we should invalidate x17 memoryTempRegister here).
     19            0xedf8d4fc4: mov    w16, #0x1b
     20            0xedf8d4fc8: orr    w16, w17, w16
     21            0xedf8d4fcc: strh   w16, [x17]                // x17 memoryTempRegister is reused while its content is invalid.
     22
     23        The problem is that `load` and `store` functions are not supporting datasize = 16 case. This patch extends `MacroAssemblerARM64::{load,store}`
     24        to support 16 so that `or16` implementation looks is similar to `or32` etc.
     25
     26        * assembler/MacroAssemblerARM64.h:
     27        (JSC::MacroAssemblerARM64::load16):
     28        (JSC::MacroAssemblerARM64::store16):
     29        (JSC::MacroAssemblerARM64::load):
     30        (JSC::MacroAssemblerARM64::store):
     31        * assembler/testmasm.cpp:
     32        (JSC::testOrImmMem):
     33
    1342019-11-20  Saam Barati  <sbarati@apple.com>
    235

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h

    r252699 r252728  
    12551255    void load16(const void* address, RegisterID dest)
    12561256    {
    1257         moveToCachedReg(TrustedImmPtr(address), cachedMemoryTempRegister());
    1258         m_assembler.ldrh(dest, memoryTempRegister, 0);
     1257        load<16>(address, dest);
    12591258    }
    12601259
    … …  
    15691568    void store16(RegisterID src, const void* address)
    15701569    {
    1571         moveToCachedReg(TrustedImmPtr(address), cachedMemoryTempRegister());
    1572         m_assembler.strh(src, memoryTempRegister, 0);
     1570        store<16>(src, address);
    15731571    }
    15741572
    … …  
    42214219            if (isInt<32>(addressDelta)) {
    42224220                if (Assembler::canEncodeSImmOffset(addressDelta)) {
    4223                     m_assembler.ldur<datasize>(dest, memoryTempRegister, addressDelta);
     4221                    loadUnscaledImmediate<datasize>(dest, memoryTempRegister, addressDelta);
    42244222                    return;
    42254223                }
    42264224
    42274225                if (Assembler::canEncodePImmOffset<datasize>(addressDelta)) {
    4228                     m_assembler.ldr<datasize>(dest, memoryTempRegister, addressDelta);
     4226                    loadUnsignedImmediate<datasize>(dest, memoryTempRegister, addressDelta);
    42294227                    return;
    42304228                }
    … …  
    42344232                m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
    42354233                cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
    4236                 m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
     4234                if constexpr (datasize == 16)
     4235                    m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr);
     4236                else
     4237                    m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
    42374238                return;
    42384239            }
    … …  
    42444245        else
    42454246            cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
    4246         m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
     4247        if constexpr (datasize == 16)
     4248            m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr);
     4249        else
     4250            m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
    42474251    }
    42484252
    … …  
    42584262            if (isInt<32>(addressDelta)) {
    42594263                if (Assembler::canEncodeSImmOffset(addressDelta)) {
    4260                     m_assembler.stur<datasize>(src, memoryTempRegister, addressDelta);
     4264                    storeUnscaledImmediate<datasize>(src, memoryTempRegister, addressDelta);
    42614265                    return;
    42624266                }
    42634267
    42644268                if (Assembler::canEncodePImmOffset<datasize>(addressDelta)) {
    4265                     m_assembler.str<datasize>(src, memoryTempRegister, addressDelta);
     4269                    storeUnsignedImmediate<datasize>(src, memoryTempRegister, addressDelta);
    42664270                    return;
    42674271                }
    … …  
    42714275                m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
    42724276                cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
    4273                 m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
     4277                if constexpr (datasize == 16)
     4278                    m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr);
     4279                else
     4280                    m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
    42744281                return;
    42754282            }
    … …  
    42784285        move(TrustedImmPtr(address), memoryTempRegister);
    42794286        cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
    4280         m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
     4287        if constexpr (datasize == 16)
     4288            m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr);
     4289        else
     4290            m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
    42814291    }
    42824292

  • trunk/Source/JavaScriptCore/assembler/testmasm.cpp

    r252422 r252728  
    11281128    invoke<void>(or16);
    11291129    CHECK_EQ(memoryLocation, 0x12341234 | 42);
     1130
     1131    memoryLocation = 0x12341234;
     1132    auto or16InvalidLogicalImmInARM64 = compile([&] (CCallHelpers& jit) {
     1133        emitFunctionPrologue(jit);
     1134        jit.or16(CCallHelpers::TrustedImm32(0), CCallHelpers::AbsoluteAddress(&memoryLocation));
     1135        emitFunctionEpilogue(jit);
     1136        jit.ret();
     1137    });
     1138    invoke<void>(or16InvalidLogicalImmInARM64);
     1139    CHECK_EQ(memoryLocation, 0x12341234);
    11301140}
    11311141
Note: See TracChangeset for help on using the changeset viewer.