[webkit-reviews] review granted: [Bug 174697] Add the ability to change sp and pc to the ARM64 JIT probe. : [Attachment 316425] proposed patch.

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 174697: Add the ability to change sp and pc to the ARM64 JIT probe.
https://bugs.webkit.org/show_bug.cgi?id=174697

Attachment 316425: proposed patch.

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




--- Comment #6 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 316425
  --> https://bugs.webkit.org/attachment.cgi?id=316425
proposed patch.

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:239
> +#define IN_ERROR_FUNCTION_OFFSET   (6 * PTR_SIZE)

Wouldn't offsetof be more natural to use instead of the macros? Especially
given the static_assert below. Or is this because you're passing it to the
assembler?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:282
> +    ".align 4" "\n"

.align is ambiguous between assemblers. .balign and .p2align are better IMO.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:304
> +    "mrs	  x1, fpsr" "\n" // Preload fpsr.

I think you should issue one of the two mrs right after the stp x0, x1, and the
other a few cycles later.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:316
> +    "stp	  x22, x23,[sp, #" STRINGIZE_VALUE_OF(PROBE_CPU_X22_OFFSET) "]"
"\n"

Missing a space before the bracket.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:324
> +    "str	  lr, [sp, #" STRINGIZE_VALUE_OF(SAVED_PROBE_RETURN_PC_OFFSET)
"]" "\n"

Can the above two be stp lr, x6 [sp, PC_OFFSET] ?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:333
> +    "add	  x9, sp, #" STRINGIZE_VALUE_OF(PROBE_CPU_Q0_OFFSET) "\n"

x9 is only used for indexing, right? Can you add Q0_OFFSET to sp below instead
of using x9?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:476
> +    "beq	" LOCAL_LABEL_STRING(ctiMasmProbeTrampolineEnd) "\n"

Here you can use cbz / cbnz.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:478
> +    // In order to restore lr, we need to do the restorationation at the
probe return site.

"restorationation"

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:518
> +    "add	  sp, sp, #" STRINGIZE_VALUE_OF(OUT_SIZE) "\n"

Here I think you could have used incrementing ldp instead of adding at the end.