[webkit-reviews] review granted: [Bug 191548] Enable JIT on ARM/Linux : [Attachment 355322] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has granted Dominik Inführ
<dinfuehr at igalia.com>'s request for review:
Bug 191548: Enable JIT on ARM/Linux
https://bugs.webkit.org/show_bug.cgi?id=191548

Attachment 355322: Patch

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




--- Comment #5 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 355322
  --> https://bugs.webkit.org/attachment.cgi?id=355322
Patch

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

r=me with comments.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:298
> +    if (compileCallEval(bytecode)) {

This brace is not necessary.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:628
> +elsif ARMv7
> +    macro probe(action)
> +	   push a0
> +	   push a1
> +	   push a2
> +	   push a3
> +	   push t0
> +	   push t1
> +	   push t2
> +	   push t3
> +	   push t4
> +	   push t5
> +	   push lr
> +	   push csr0
> +
> +	   action()
> +
> +	   pop csr0
> +	   pop lr
> +	   pop t5
> +	   pop t4
> +	   pop t3
> +	   pop t2
> +	   pop t1
> +	   pop t0
> +	   pop a3
> +	   pop a2
> +	   pop a1
> +	   pop a0
> +    end

The implementation is almost the same to the above implementation for X86_64 or
ARM64 or ARM64E.
Can we share the code with it?

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:447
> +
> +    if (!codeBlock->instructions().size()) {
> +	   isValid = false;
> +	   return 0;
> +    }

I think this is unnecessary if we add InstructionStream::contains, which is
described below.

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:451
> +    const Instruction* begin = codeBlock->instructions().at(0).ptr();
> +    const Instruction* end = reinterpret_cast<const
Instruction*>(reinterpret_cast<const uint8_t*>(begin) +
codeBlock->instructions().size());
> +    if (instruction >= begin && instruction < end) {

I think this should be encapsulated in InstructionStream class. Adding
InstructionStream::contains, and

if (codeBlock->instructions().contains(nstruction)) {

looks better.

> Source/cmake/WebKitFeatures.cmake:71
>      elseif (WTF_CPU_ARM AND WTF_OS_UNIX)

Should we switch this `UNIX` to `LINUX` because `OS(LINUX)` is checked in
Platform.h?