From 30ba860d7bae967717888ce2b97513e84bc4f897 Mon Sep 17 00:00:00 2001 From: "ysuzuki@apple.com" Date: Thu, 21 Nov 2019 06:02:50 +0000 Subject: [PATCH] [JSC] Extend MacroAssemblerARM64::load/store for datasize = 16 https://bugs.webkit.org/show_bug.cgi?id=204442 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 --- Source/JavaScriptCore/ChangeLog | 33 ++++++++++++++++++ .../assembler/MacroAssemblerARM64.h | 34 ++++++++++++------- Source/JavaScriptCore/assembler/testmasm.cpp | 10 ++++++ 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index 0e429dbb4cbde..5c7763aa9c32d 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,36 @@ +2019-11-20 Yusuke Suzuki + + [JSC] Extend MacroAssemblerARM64::load/store for datasize = 16 + https://bugs.webkit.org/show_bug.cgi?id=204442 + + + 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 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 index dbff2d71761fc..3fe354ee751c2 100644 --- a/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h +++ b/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h @@ -1254,8 +1254,7 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { 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 { 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,12 +4218,12 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { if (isInt<32>(addressDelta)) { if (Assembler::canEncodeSImmOffset(addressDelta)) { - m_assembler.ldur(dest, memoryTempRegister, addressDelta); + loadUnscaledImmediate(dest, memoryTempRegister, addressDelta); return; } if (Assembler::canEncodePImmOffset(addressDelta)) { - m_assembler.ldr(dest, memoryTempRegister, addressDelta); + loadUnsignedImmediate(dest, memoryTempRegister, addressDelta); return; } } @@ -4233,7 +4231,10 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) { m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0); cachedMemoryTempRegister().setValue(reinterpret_cast(address)); - m_assembler.ldr(dest, memoryTempRegister, ARM64Registers::zr); + if constexpr (datasize == 16) + m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr); + else + m_assembler.ldr(dest, memoryTempRegister, ARM64Registers::zr); return; } } @@ -4243,7 +4244,10 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { cachedMemoryTempRegister().invalidate(); else cachedMemoryTempRegister().setValue(reinterpret_cast(address)); - m_assembler.ldr(dest, memoryTempRegister, ARM64Registers::zr); + if constexpr (datasize == 16) + m_assembler.ldrh(dest, memoryTempRegister, ARM64Registers::zr); + else + m_assembler.ldr(dest, memoryTempRegister, ARM64Registers::zr); } template @@ -4257,12 +4261,12 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { if (isInt<32>(addressDelta)) { if (Assembler::canEncodeSImmOffset(addressDelta)) { - m_assembler.stur(src, memoryTempRegister, addressDelta); + storeUnscaledImmediate(src, memoryTempRegister, addressDelta); return; } if (Assembler::canEncodePImmOffset(addressDelta)) { - m_assembler.str(src, memoryTempRegister, addressDelta); + storeUnsignedImmediate(src, memoryTempRegister, addressDelta); return; } } @@ -4270,14 +4274,20 @@ class MacroAssemblerARM64 : public AbstractMacroAssembler { if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) { m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0); cachedMemoryTempRegister().setValue(reinterpret_cast(address)); - m_assembler.str(src, memoryTempRegister, ARM64Registers::zr); + if constexpr (datasize == 16) + m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr); + else + m_assembler.str(src, memoryTempRegister, ARM64Registers::zr); return; } } move(TrustedImmPtr(address), memoryTempRegister); cachedMemoryTempRegister().setValue(reinterpret_cast(address)); - m_assembler.str(src, memoryTempRegister, ARM64Registers::zr); + if constexpr (datasize == 16) + m_assembler.strh(src, memoryTempRegister, ARM64Registers::zr); + else + m_assembler.str(src, memoryTempRegister, ARM64Registers::zr); } template diff --git a/Source/JavaScriptCore/assembler/testmasm.cpp b/Source/JavaScriptCore/assembler/testmasm.cpp index 59bbc277fadcd..f11332f73bd63 100644 --- a/Source/JavaScriptCore/assembler/testmasm.cpp +++ b/Source/JavaScriptCore/assembler/testmasm.cpp @@ -1127,6 +1127,16 @@ void testOrImmMem() }); invoke(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(or16InvalidLogicalImmInARM64); + CHECK_EQ(memoryLocation, 0x12341234); } void testByteSwap()