[Webkit-unassigned] [Bug 44329] SH4 JIT SUPPORT
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 15 03:09:22 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44329
--- Comment #22 from thouraya <thouraya.andolsi at st.com> 2010-10-15 03:09:22 PST ---
Hello,
(In reply to comment #21)
> > 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?
We started working on webkit in 2009 and we are updating it from the trunk each time.
At that time there was some calls directly to X86 registers, so I prefered to create a customized file for SH4 in order to optimize a bit.
The port is for 32-bit.
We can work on 64-bit , after the 32-bit port is accepted.
>
> > 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.
We started working on Webkit JIT in 2009, but it was available in 2010.
> 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. ;)
yes of course :)
>
> > 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?
The common solution should work fine, but with our solution we reduce the number of instructions.
DT is equivalent to subtract one and compare.
>
> > 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!
The offset of branch instruction may be huge, so we need to load it in the constantPool especially when jumping to slowcases.
In other cases the offset is reachable and we dont need to load it, that's why I added this parameter.
>
> > 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.
In the common code, there are a lot of branches and the offsets are known when linking.
when branching to solwcase, the offset is not reachable, so we need to load it in the constantPool, otherwise there is no need. that's why I added in branch functions a parameter to check if we need to load a constant or not.
Removing the changes done in the common code, webkit should work fine in SH4,
but there are a lot of constants loaded in the constantpool in banch instructions but offsets are reached.
Regards,
Thouraya.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list