Bug 213103 – JIT thunks should work on arm64_32

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

JIT thunks should work on arm64_32

[Keith Miller]

Created attachment 401694 [details]
Patch

[Keith Miller]

Created attachment 401752 [details]
Patch

[Keith Miller]

Created attachment 401755 [details]
Patch

[Keith Miller]

Created attachment 401768 [details]
Patch

[Darin Adler]

Comment on attachment 401768 [details]
Patch

Looks just right to me, but I probably shouldn't review because I’m not familiar enough with this.

[Saam Barati]

Comment on attachment 401768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401768&action=review

r=me

> Source/JavaScriptCore/ChangeLog:16
> +        2) MacroAssembler::*Ptr functions call 32/64 bit variants based on
> +        Address space size rather than cpu architecture.

What about call sites where they're using Ptr as a shortcut for 64-bit? Will those be vetted and changed later on?

> Source/JavaScriptCore/ChangeLog:23
> +        5) numberOfDFGCompiles should report a big number for useBaselineJIT=0

you should say why

> Source/JavaScriptCore/assembler/MacroAssembler.h:1253
> +        bool shouldBlindDouble(double value)

this indentation looks off

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:155
> +        if (b == ARM64Registers::sp)
> +            std::swap(a, b);

is this required by the architecture?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2510
>      void signExtend32ToPtr(TrustedImm32 imm, RegisterID dest)
>      {
> -        move(TrustedImmPtr(reinterpret_cast<void*>(static_cast<intptr_t>(imm.m_value))), dest);
> +        move(TrustedImm64(imm.m_value), dest);
>      }

Why is this needed for address32? Shouldn't we doing a 32-bit-mov?

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:447
> +        if (!imm.m_value)
> +            move(src, dest);
> +        else
> +            m_assembler.ror(dest, src, imm.m_value & 0x1f);

do these have the same semantics for what happen to upper bits?

[Keith Miller]

Comment on attachment 401768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401768&action=review

>> Source/JavaScriptCore/ChangeLog:16
>> +        Address space size rather than cpu architecture.
> 
> What about call sites where they're using Ptr as a shortcut for 64-bit? Will those be vetted and changed later on?

Yeah, I'm going through those as I go. I couldn't think of an automated way of checking for this with the way our current code is laid out. Ideally, we'd have something that returned the sizeof(<member>) in all our offsetOf* functions so we can assert it's the correct size.

>> Source/JavaScriptCore/ChangeLog:23
>> +        5) numberOfDFGCompiles should report a big number for useBaselineJIT=0
> 
> you should say why

Sure.

>> Source/JavaScriptCore/assembler/MacroAssembler.h:1253
>> +        bool shouldBlindDouble(double value)
> 
> this indentation looks off

indeed, will fix.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:155
>> +            std::swap(a, b);
> 
> is this required by the architecture?

Not sure, I just copied the code for add64... I didn't vet it in detail.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2510
>>      }
> 
> Why is this needed for address32? Shouldn't we doing a 32-bit-mov?

You sometimes need to signExtend to the full register size as the instruction operates on the full 64-bit operand. This should likely be renamed to signExtendToRegister as that is how we use for the most part but identifying which spots are which is hard.

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:447
>> +            m_assembler.ror(dest, src, imm.m_value & 0x1f);
> 
> do these have the same semantics for what happen to upper bits?

Huh? This is only for armv7? What upper bits are you talking about?

[Keith Miller]

Created attachment 401912 [details]
Patch for landing

[EWS]

Committed r263049: <https://trac.webkit.org/changeset/263049>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401912 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64373326>