[webkit-reviews] review denied: [Bug 172767] [JSC][ARMv6] Fix ARMv6 JIT support : [Attachment 314551] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 172767: [JSC][ARMv6] Fix ARMv6 JIT support
https://bugs.webkit.org/show_bug.cgi?id=172767

Attachment 314551: Patch

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




--- Comment #13 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 314551
  --> https://bugs.webkit.org/attachment.cgi?id=314551
Patch

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

I think this patch is almost ready now that we have the spec so that we know
it's doing the right thing.  Please apply the changes.	r- for now.  Thanks.

> Source/JavaScriptCore/ChangeLog:8
> +	   Original author is Guillaume Emont.

Any specific reason why Guillaume isn't posting the patch himself?

> Source/JavaScriptCore/assembler/ARMAssembler.h:217
> +	       // mcr	  15, 0, r6, cr7, cr10, {5}

I think it's helpful to add a comment here saying:
    // The r6 argument register is ignored by this operation. Hence, it does
not need to be initialized.
    // See section B6.2.2 in page B6-1945 of
https://static.docs.arm.com/ddi0406/c/DDI0406C_C_arm_architecture_reference_man
ual.pdf.

It also seems quite arbitrary and out of left field to choose r6 (as if there's
some special reason for its choice).  Why not just choose r0?

> Source/JavaScriptCore/assembler/ARMAssembler.h:218
> +	       ARM6_MEMFENCE = 0xee076fba,

The spec for the in section B6.2.2 page B6-1946 of
https://static.docs.arm.com/ddi0406/c/DDI0406C_C_arm_architecture_reference_man
ual.pdf says:
"CP15DMB, Data Memory Barrier operation
In ARMv7, use the DMB instruction to perform a Data Memory Barrier, see DMB on
page A8-378.
The deprecated CP15 c7 encoding for a Data Memory Barrier is an MCR instruction
with <opc1> set to 0, <CRn> set to c7, <CRm> set to c10, and <opc2> set to 5.
This operation performs the full system barrier performed by the DMB
instruction."

Note the part that says "full system barrier performed by the DMB instruction".

Hence, let's rename ARM6_MEMFENCE as ARM6_DMB_SY.

> Source/JavaScriptCore/assembler/ARMAssembler.h:733
> -	   void dmbSY()
> +	   void memoryFence()

Let's undo this.

> Source/JavaScriptCore/assembler/ARMAssembler.h:738
> +	       m_buffer.putInt(ARM6_MEMFENCE);

Rename to ARM6_DMB_SY.

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1489
> -	   m_assembler.dmbSY();
> +	   m_assembler.memoryFence();

Undo this.