[Webkit-unassigned] [Bug 44329] SH4 JIT SUPPORT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44329





--- Comment #21 from Gabor Loki <loki at webkit.org>  2010-10-15 01:24:50 PST ---
> JavaScriptCore/GNUmakefile.am:544
> +if CPU_SH4
> +javascriptcore_sources += \
> +    JavaScriptCore/jit/JITArithmetic_sh4.cpp
> +else
> +javascriptcore_sources += \
> +    JavaScriptCore/jit/JITArithmetic.cpp
> +endif

Is it really necessary? And what is about JSValue32_64?

> JavaScriptCore/assembler/MacroAssemblerSH4.h:5
> + * Copyright (C) 2009-2010 STMicroelectronics. All rights reserved.
> + * Written by Thouraya Andolsi (thouraya.andolsi at st.com).
> + *
> + * Copyright (C) 2008 Apple Inc. All rights reserved.

It is possible that you had SH4 JIT since 2009, but the public version is just released in this year.
The "Written by" line should be another Copyright line instead (for example: Copyright (C) 2010 Thouraya Andolsi <thouraya.andolsi at st.com>), and the Copyright lines should be in ascending time order.

> JavaScriptCore/assembler/SH4Assembler.cpp:22
> +void* SH4Assembler::executableCopy(ExecutablePool* allocator)
> +{
> +    void* copy = m_buffer.executableCopy(allocator);
> +
> +    ASSERT(copy);
> +    return copy;
> +}

If this executableCopy just calls its buffer's executableCopy, you should move this function as an inline function into the header.

> JavaScriptCore/assembler/SH4Assembler.h:38
> +#define GETOPCODE1(a, b, c)  ((a) | (((b) & 0xf) << 8) |(((c) & 0xf) << 4))
> +#define GETOPCODE2(a, b)  ((a) | (((b) & 0xf) << 8))

Please, use inline functions and better names (see the formatter of Thumb-2 JIT).

> JavaScriptCore/assembler/SH4Assembler.h:54
> +#define INVALID_OPCODE   0xffff
> +#define ADD_OPCODE       0x300C
> +#define ADDIMM_OPCODE    0x7000
> +#define ADDC_OPCODE      0x300E
> +#define ADDV_OPCODE      0x300F

It would be better using enumerations instead of defines.

> JavaScriptCore/assembler/SH4Assembler.h:270
> +        R14 = 14,
> +        R15 = 15,
> +        SP  = R15,
> +        FP  = R14,

There is no need for all initializers. See:
R0
...
R13,
R14, FP = R14,
R15, SP = R15,
PC,
PR

> JavaScriptCore/assembler/SH4Assembler.h:394
> +        if ((constant <=127) && (constant >= -128))

There is a missing space before 127 constant. The inner parentheses are not needed.

> JavaScriptCore/assembler/SH4Assembler.h:521
> +        ASSERT((imm8 <=127) && (imm8 >= -128));

Again, missing space before 127 and parentheses.

> JavaScriptCore/assembler/SH4Assembler.h:708
> +        oneShortOp(opc);
> +
> +    }

There is unnecessary empty line at the end of a function.

> JavaScriptCore/assembler/SH4Assembler.h:751
> +        case HS: opc = GETOPCODE1(CMPHS_OPCODE, right, left); // HS

I guess this comment is not necessary. ;)

> JavaScriptCore/assembler/SH4Assembler.h:1527
> +            instruction = (0x0023 | (*instructionPtr++ & 0xF00));

We prefer enumerations and static integers instead of magic numbers. Well, it is not necessary for every cases, but it can help to understand the code (especially when you are altering an instruction).

> JavaScriptCore/assembler/SH4Assembler.h:1540
> +            uint16_t* address  = (uint16_t*)((offsetToPC << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));

Use C++ like type conversion (static_cast<>, reinterpret_cast<>, etc.) instead of this C cast!

> JavaScriptCore/assembler/SH4Assembler.h:1763
> +         } else  
> +            ASSERT_NOT_REACHED();

If it is true, you should remove the last 'if' statement and use an ASSERT inside the last (else) block instead of this solution.
For example:
if () {
} else if () {
} else {
  ASSERT(cond);
}

> JavaScriptCore/assembler/SH4Assembler.h:1838
> +        /*
> +        code@  mov.l @(offset , PC) , reg
> +    */  

There is a white-space problem in this comment.

> JavaScriptCore/jit/JIT.cpp:121
>  #else
> +#if CPU(SH4)
> +void JIT::emitTimeoutCheck()
> +{
> +    m_assembler.dt(timeoutCheckRegister);
> +
> +    Jump skipTimeout = Jump(m_assembler.jne());
> +    m_assembler.nop();
> +    m_assembler.nop();
> +    JITStubCall(this, cti_timeout_check).call(timeoutCheckRegister);
> +    skipTimeout.link(this);
> +
> +    killLastResultRegister();
> +}
> +#else
>  void JIT::emitTimeoutCheck()

Why is it necessary? What is SH4's problem about the common solution?

> JavaScriptCore/jit/JIT.h:475
>          void emitJumpSlowCaseIfJSCell(RegisterID);
> +#if CPU(SH4)
> +        Jump emitJumpIfNotJSCell(RegisterID, bool needConstant = true);
> +#else
>          Jump emitJumpIfNotJSCell(RegisterID);
> +#endif
>          void emitJumpSlowCaseIfNotJSCell(RegisterID);

Could you tell us more why these kind of changes are necessary in the common interface? So many changes in the common code!

> JavaScriptCore/jit/JITArithmetic_sh4.cpp:32
> +extern unsigned int __fpscr_values[2];

Hmm, where is the initializer of this? Is it some kind of static array? And what will you do with it if there is multiple JSC thread?


Well, the assembler part of this patch is fine (with some cosmetic changes), but I do not support those *big* changes in the common interface of JIT. I do not think that it is impossible to handle those problems with jumps and literals in the current macroassembler interface. I know the macroassembler is a little bit x86 specific, but ARM (with ARM and Thumb-2 instruction set) and MIPS can also fit this infrastructure well. So, I encourage you to rewrite the MacroAssemblerSH4 interface to reduce the current changes in the common interface. However, Thouraya, you did a great work to implement SH4 JIT, but I think your patch is not acceptable in the current form.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.