Enable JIT on ARM/Linux

• Component: New Bugs
• Status: RESOLVED FIXED

[Dominik Inführ]

Enable JIT on ARM/Linux

[Dominik Inführ]

Created attachment 354562 [details]
Patch

[Dominik Inführ]

This patch enables the baseline JIT on ARM/Linux. It also contains patch https://bugs.webkit.org/show_bug.cgi?id=191256, since that hasn't been reviewed yet. It probably makes sense to land them separately.

[Yusuke Suzuki]

Could you rebase the patch?

[Dominik Inführ]

Created attachment 355322 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 355322 [details]
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?

[Dominik Inführ]

Created attachment 355356 [details]
Patch

[Dominik Inführ]

Thanks for the review! I think I've addressed your comments.

The braces are removed. The probe-macro from x64 and arm64 is now also used for arm. I added InstructionStream::contains and in CMakeLists.txt I am now using WTF_OS_LINUX instead of WTF_OS_UNIX (I had to introduce that flag).

[Yusuke Suzuki]

Comment on attachment 355356 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/InstructionStream.cpp:49
> +    const Instruction* begin = at(0).ptr();
> +    const Instruction* end = reinterpret_cast<const Instruction*>(reinterpret_cast<const uint8_t*>(begin) + size());
> +
> +    return instruction >= begin && instruction < end;

Actually, you do not need to use `Instruction*` here. You can define this function simply by,

const uint8_t* pointer = bitwise_cast<const uint8_t*>(instruction);
return pointer >= m_instructions.data() && pointer < (m_instructions.data() + m_instructions.size());

> Source/JavaScriptCore/jit/GPRInfo.h:531
> +#define NUMBER_OF_CALLEE_SAVES_REGISTERS 1u

This is only correct for ARM_THUMB2.

[Dominik Inführ]

Created attachment 355395 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 355395 [details]
Patch

r=me, nice!

[WebKit Commit Bot]

Comment on attachment 355395 [details]
Patch

Clearing flags on attachment: 355395

Committed r238414: <https://trac.webkit.org/changeset/238414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46198694>