[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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@252728 268f45cc-cd09-0410-ab3c-d52691b4dbfc
haiku · Nov 21, 2019 · 30ba860 · 30ba860
1 parent 9b1acad
commit 30ba860
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 12 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,36 @@
+2019-11-20  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [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):
+
 2019-11-20  Saam Barati  <sbarati@apple.com>
 
         Baseline JIT should fill in StructureStubInfo's propertyIsInt32 and the slow path should update the array profile more frequently

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h b/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h
@@ -1254,8 +1254,7 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler<Assembler> {
 
     void load16(const void* address, RegisterID dest)
     {
-        moveToCachedReg(TrustedImmPtr(address), cachedMemoryTempRegister());
-        m_assembler.ldrh(dest, memoryTempRegister, 0);
+        load<16>(address, dest);
     }
 
     void load16Unaligned(ImplicitAddress address, RegisterID dest)
@@ -1568,8 +1567,7 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler<Assembler> {
 
     void store16(RegisterID src, const void* address)
     {
-        moveToCachedReg(TrustedImmPtr(address), cachedMemoryTempRegister());
-        m_assembler.strh(src, memoryTempRegister, 0);
+        store<16>(src, address);
     }
 
     void store16(TrustedImm32 imm, const void* address)
@@ -4220,20 +4218,23 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler<Assembler> {
 
             if (isInt<32>(addressDelta)) {
                 if (Assembler::canEncodeSImmOffset(addressDelta)) {
-                    m_assembler.ldur<datasize>(dest,  memoryTempRegister, addressDelta);
+                    loadUnscaledImmediate<datasize>(dest, memoryTempRegister, addressDelta);
                     return;
                 }
 
                 if (Assembler::canEncodePImmOffset<datasize>(addressDelta)) {
-                    m_assembler.ldr<datasize>(dest,  memoryTempRegister, addressDelta);
+                    loadUnsignedImmediate<datasize>(dest, memoryTempRegister, addressDelta);
                     return;
                 }
             }
 
             if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) {
                 m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
                 cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
-                m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
+                if constexpr (datasize == 16)
+                    m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr);
+                else
+                    m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
                 return;
             }
         }
@@ -4243,7 +4244,10 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler<Assembler> {
             cachedMemoryTempRegister().invalidate();
         else
             cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
-        m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
+        if constexpr (datasize == 16)
+            m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr);
+        else
+            m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
     }
 
     template<int datasize>
@@ -4257,27 +4261,33 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler<Assembler> {
 
             if (isInt<32>(addressDelta)) {
                 if (Assembler::canEncodeSImmOffset(addressDelta)) {
-                    m_assembler.stur<datasize>(src, memoryTempRegister, addressDelta);
+                    storeUnscaledImmediate<datasize>(src, memoryTempRegister, addressDelta);
                     return;
                 }
 
                 if (Assembler::canEncodePImmOffset<datasize>(addressDelta)) {
-                    m_assembler.str<datasize>(src, memoryTempRegister, addressDelta);
+                    storeUnsignedImmediate<datasize>(src, memoryTempRegister, addressDelta);
                     return;
                 }
             }
 
             if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) {
                 m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
                 cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
-                m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
+                if constexpr (datasize == 16)
+                    m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr);
+                else
+                    m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
                 return;
             }
         }
 
         move(TrustedImmPtr(address), memoryTempRegister);
         cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
-        m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
+        if constexpr (datasize == 16)
+            m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr);
+        else
+            m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
     }
 
     template <int dataSize>

diff --git a/Source/JavaScriptCore/assembler/testmasm.cpp b/Source/JavaScriptCore/assembler/testmasm.cpp
@@ -1127,6 +1127,16 @@ void testOrImmMem()
     });
     invoke<void>(or16);
     CHECK_EQ(memoryLocation, 0x12341234 | 42);
+
+    memoryLocation = 0x12341234;
+    auto or16InvalidLogicalImmInARM64 = compile([&] (CCallHelpers& jit) {
+        emitFunctionPrologue(jit);
+        jit.or16(CCallHelpers::TrustedImm32(0), CCallHelpers::AbsoluteAddress(&memoryLocation));
+        emitFunctionEpilogue(jit);
+        jit.ret();
+    });
+    invoke<void>(or16InvalidLogicalImmInARM64);
+    CHECK_EQ(memoryLocation, 0x12341234);
 }
 
 void testByteSwap()