[webkit-reviews] review granted: [Bug 213103] JIT thunks should work on arm64_32 : [Attachment 401768] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 213103: JIT thunks should work on arm64_32
https://bugs.webkit.org/show_bug.cgi?id=213103

Attachment 401768: Patch

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




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 401768
  --> https://bugs.webkit.org/attachment.cgi?id=401768
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?