[webkit-reviews] review granted: [Bug 233474] [JSC] Generated code size reductions for baseline JIT (all architectures) : [Attachment 445419] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 1 09:32:06 PST 2021


Yusuke Suzuki <ysuzuki at apple.com> has granted Geza Lore <glore at igalia.com>'s
request for review:
Bug 233474: [JSC] Generated code size reductions for baseline JIT (all
architectures)
https://bugs.webkit.org/show_bug.cgi?id=233474

Attachment 445419: Patch

https://bugs.webkit.org/attachment.cgi?id=445419&action=review




--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 445419
  --> https://bugs.webkit.org/attachment.cgi?id=445419
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-Architectur
e/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.


More information about the webkit-reviews mailing list