Bug 233474 – [JSC] Generated code size reductions for baseline JIT (all architectures)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geza Lore]

[JSC] Code size improvements for Baseline and ARMv7/Thumb-2

[Geza Lore]

Created attachment 445097 [details]
Patch

[Geza Lore]

Created attachment 445266 [details]
Patch

[Geza Lore]

Created attachment 445419 [details]
Patch

[Yusuke Suzuki]

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.

[Radar WebKit Bug Importer]

<rdar://problem/85926098>

[Geza Lore]

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!

[Geza Lore]

Created attachment 445699 [details]
Patch

[EWS]

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].

[Saam Barati]

Nice, this seems like it could be a speedup on Speedometer2.

[Saam Barati]

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"

[Saam Barati]

(In reply to Saam Barati from comment #9)
> Nice, this seems like it could be a speedup on Speedometer2.

around 1% on some devices.

[Geza Lore]

Thanks for letting me know, that is great to hear. (wish those perf bots were public!)