Summary: | [JSC] Generated code size reductions for baseline JIT (all architectures) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geza Lore <glore> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Geza Lore
2021-11-24 09:38:51 PST
Created attachment 445097 [details]
Patch
Created attachment 445266 [details]
Patch
Created attachment 445419 [details]
Patch
Comment on attachment 445419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445419&action=review Nice. r=me. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:1268 > + ASSERT(!(offset & 0x3)); According to ARM Architecture Reference Manual, https://developer.arm.com/documentation/ddi0406/b/Application-Level-Architecture/Instruction-Details/Alphabetical-list-of-instructions/LDRD--register-?lang=en <Rt> The first destination register. This register must be even-numbered and not R14. <Rt2> The second destination register. This register must be <R(t+1)>. So, let's add assertions for the above condition. ASSERT(rt2 == rt + 1); ASSERT(rt % 2 == 0); ASSERT(rt != r14); > Source/JavaScriptCore/assembler/ARMv7Assembler.h:1745 > + ASSERT(!(offset & 0x3)); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:62 > + RegisterID scratchRegister() { return addressTempRegister; } Can you review all places using this scratchRegister? I think it is possible that some places are using addressTempRegister and scratchRegister(), and in that case, this change breaks it. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:872 > + if (!(absOffset & ~0x3fc)) > + m_assembler.ldrd(dest1, dest2, address.base, address.offset, /* index: */ true, /* wback: */ false); According to ARM Architecture Reference Manual, we need to ensure that, dest1 + 1 == dest2 dest1 != r14 > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1009 > + m_assembler.strd(src1, src2, address.base, address.offset, /* index: */ true, /* wback: */ false); Ditto. > Source/JavaScriptCore/jit/AssemblyHelpers.h:207 > + static_assert((PayloadOffset == 4 && !TagOffset) || (!PayloadOffset && TagOffset == 4)); > + if constexpr (PayloadOffset < TagOffset) > + loadPair32(address, regs.payloadGPR(), regs.tagGPR()); > + else > + loadPair32(address, regs.tagGPR(), regs.payloadGPR()); Ditto. > Source/JavaScriptCore/jit/AssemblyHelpers.h:216 > + static_assert((PayloadOffset == 4 && !TagOffset) || (!PayloadOffset && TagOffset == 4)); You can insert static_assert that, static_assert(!PayloadOffset && TagOffset == 4) because we never enable JIT on BigEndian environments. > Source/JavaScriptCore/jit/AssemblyHelpers.h:220 > + if constexpr (PayloadOffset < TagOffset) > + loadPair32(address, regs.payloadGPR(), regs.tagGPR()); > + else > + loadPair32(address, regs.tagGPR(), regs.payloadGPR()); And we can just use loadPair32(address, regs.payloadGPR(), regs.tagGPR()); > Source/JavaScriptCore/jit/JITOpcodes.cpp:1260 > - for (size_t j = CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters(); j < count; ++j) > - emitInitRegister(virtualRegisterForLocal(j)); > + size_t first = CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters(); > + if (first < count) > + moveTrustedValue(jsUndefined(), jsRegT10); > + for (size_t j = first; j < count; ++j) > + emitPutVirtualRegister(virtualRegisterForLocal(j), jsRegT10); Probably, we should use ENABLE(EXTRA_CTI_THUNKS) path in all architectures. But for now, it is OK since op_enter_handlerGenerator assumes 64bit right now. Comment on attachment 445419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445419&action=review >> Source/JavaScriptCore/assembler/ARMv7Assembler.h:1268 >> + ASSERT(!(offset & 0x3)); > > According to ARM Architecture Reference Manual, https://developer.arm.com/documentation/ddi0406/b/Application-Level-Architecture/Instruction-Details/Alphabetical-list-of-instructions/LDRD--register-?lang=en > <Rt> > The first destination register. This register must be even-numbered and not R14. > > <Rt2> > The second destination register. This register must be <R(t+1)>. > > So, let's add assertions for the above condition. > ASSERT(rt2 == rt + 1); > ASSERT(rt % 2 == 0); > ASSERT(rt != r14); As discussed, these constraints do not apply to the Thumb-2 instruction set. See "Encoding T1" here: https://developer.arm.com/documentation/ddi0406/b/Application-Level-Architecture/Instruction-Details/Alphabetical-list-of-instructions/LDRD--immediate-?lang=en >> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:62 >> + RegisterID scratchRegister() { return addressTempRegister; } > > Can you review all places using this scratchRegister? I think it is possible that some places are using addressTempRegister and scratchRegister(), and in that case, this change breaks it. I did review these, all current uses are fine. We should also fix https://bugs.webkit.org/show_bug.cgi?id=232373 at some point, but this change does not make the situation worse than it is. >> Source/JavaScriptCore/jit/AssemblyHelpers.h:216 >> + static_assert((PayloadOffset == 4 && !TagOffset) || (!PayloadOffset && TagOffset == 4)); > > You can insert static_assert that, > > static_assert(!PayloadOffset && TagOffset == 4) > > because we never enable JIT on BigEndian environments. Ah, great to know not having to worry about BigEndian, thanks! Created attachment 445699 [details]
Patch
Committed r286424 (244770@main): <https://commits.webkit.org/244770@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445699 [details]. Nice, this seems like it could be a speedup on Speedometer2. Comment on attachment 445699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445699&action=review > Source/JavaScriptCore/jit/JITOpcodes.cpp:1260 > + for (size_t j = first; j < count; ++j) > + emitPutVirtualRegister(virtualRegisterForLocal(j), jsRegT10); small nit: This can go in the above "if" (In reply to Saam Barati from comment #9) > Nice, this seems like it could be a speedup on Speedometer2. around 1% on some devices. Thanks for letting me know, that is great to hear. (wish those perf bots were public!) |