[Webkit-unassigned] [Bug 30144] MIPS JIT Supports
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 29 18:06:04 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=30144
Gavin Barraclough <barraclough at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #42114|review? |review-
Flag| |
--- Comment #23 from Gavin Barraclough <barraclough at apple.com> 2009-10-29 18:06:02 PDT ---
(From update of attachment 42114)
Hi Chao-ying,
First of all, many apologies for the delay in looking at this – busy couple of
weeks. This looks like a great patch.
As I think Gabor or Zoltan may have already commented, we have usually landed
new ports incrementally to try to separate new code from changes to existing
code. This both makes it easier to review, and makes it easier to track down
problems later if change were to introduce a problem. I'd suggest it would be
good to do so in this case too – it should not significantly slow down the
progress of getting this in. So if you could put up a patch for review that
reverts all changes to the JIT (and obviously also doesn't set ENABLE_JIT!)
that would be great – the first patch should just enable the YARR regex engine
on MIPS.
In the past we have also broken up the patch that enables the JIT to separately
enable the JIT without optimization, then enable the optimizations one by one.
This may not be necessary this time, since landing the last couple of ports
also required changes to make the JIT more portable – the diff needed to enable
the JIT on MIPS looks small enough that we can probably land the rest as one
piece.
> Index: wtf/Platform.h
> ===================================================================
> --- wtf/Platform.h (revision 49926)
> +++ wtf/Platform.h (working copy)
>
> @@ -225,6 +225,15 @@
> || (PLATFORM(X86_64) && PLATFORM(MAC)) \
> /* Under development, temporarily disabled until 16Mb link range limit in assembler is fixed. */ \
> || (PLATFORM(ARM_THUMB2) && PLATFORM(IPHONE) && 0) \
> - || (PLATFORM(X86) && PLATFORM(WIN))
> + || (PLATFORM(X86) && PLATFORM(WIN)) \
> + || PLATFORM(MIPS)
> #define ENABLE_YARR 1
> #define ENABLE_YARR_JIT 1
> #endif
We tend to only enable the JIT platform by platform, and only on platforms
we've tested. Also, the Linux ports don't enable features in Platform.h – I
think GTK & QT ports use the files /configure.ac and
/JavaScriptCore/JavaScriptCore.pri. What platforms have you been testing on? -
if linux, I'd suggest sticking with the other Linux ports. If other, feel free
to make the change here in Platform.h, but only do so for specific OSes.
> Index: assembler/MIPSAssembler.h
> ===================================================================
> + JmpSrc jmp()
> + {
> + m_jumps.append(m_buffer.size());
> +
> + beq(MIPSRegisters::zero, MIPSRegisters::zero, 0);
> + nop();
> + /* We need four words for relaxation. */
> + beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> + nop();
> + nop();
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
> +
> + JmpSrc branch_eq(RegisterID rs, RegisterID rt)
> + {
> + m_jumps.append(m_buffer.size());
> +
> + beq(rs, rt, 0);
> + nop();
> + /* We need four words for relaxation. */
> + beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> + nop();
> + nop();
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
> +
> + JmpSrc branch_ne(RegisterID rs, RegisterID rt)
> + {
> + m_jumps.append(m_buffer.size());
> +
> + bne(rs, rt, 0);
> + nop();
> + /* We need four words for relaxation. */
> + beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> + nop();
> + nop();
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
> +
> + JmpSrc bc1t()
> + {
> + emitInst(0x45010000);
> + nop();
> + /* We need four words for relaxation. */
> + beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> + nop();
> + nop();
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
> +
> + JmpSrc far_function_call()
> + {
> + lui(MIPSRegisters::t9, 0);
> + ori(MIPSRegisters::t9, MIPSRegisters::t9, 0);
> + jalr(MIPSRegisters::t9);
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
> +
> + JmpSrc function_call()
> + {
> + /* We need two words for relaxation. */
> + nop();
> + nop();
> + jal();
> + nop();
> + return JmpSrc(m_buffer.size());
> + }
These shouldn't really be in the Assembler layer. The goal behind the
Assembler is that this should really have a 1-to-1 mapping to the instruction
set, and the MacroAssembler should support a richer set of operations, common
across architectures. We're not entirely strict about this, and I think from a
practical perspective having the assembler automatically insert nops after
loads and coprocessor moves on MIPS1 is probably not a terrible idea – though
even this I'm not 100% comfortable about, this could be in the MacroAssembler
too. But the assembler should really be usable to plant jumps for use with
code generation other than from the MacroAssembler, and in contexts where all
code will be known to fall within a smaller region of memory, and jumps need
never be relaxed to 32-bit range. So I'd suggest these functions should really
be moved up to the MacroAssembler.
Unfortunately the layering is currently a little broken in the assemblers for
other architectures, in this area. The linking and repatching of jumps and
calls currently is handled by the Assembler in a manner that has knowledge of
the instruction sequences used by the MacroAssembler. We've started
refactoring the code to fix this, so the linking and repatching now passes
through the MacroAssemblerARCH layer, and the intention is that all knowledge
of the set of instructions used by the MacroAssembler will be captured here –
so, for example note that MacroAssemblerX86_64::linkCall is already responsible
for offseting the label to find the immediate to patch for far calls. But
cleaning all this up is clearly a work in progress.
> + void* executableCopy(ExecutablePool* allocator)
> + {
> + int size = m_buffer.size();
> + if (!size)
> + return 0;
> +
> + void* result = allocator->alloc(size);
> + if (!result)
> + return 0;
> +
> + ExecutableAllocator::makeWritable(result, size);
> + memcpy(result, m_buffer.data(), m_buffer.size());
>
^^^ This seems to just replicate functionality for
m_buffer.executableCopy(allocator); is there a reason you can't just call that?
+ relocateJumps(m_buffer.data(), result);
> + return result;
> + }
In MacroAssemblerARMv7.h ->
>> , int fixed = 0)
The one big problem I have in this patch is adding the ", int fixed = 0)"
argument to all these function. We shouldn't be adding an extra parameter to
these functions on one platform – we really need this interface to line up
across the architectures (this interface is not fixed, and if it differs across
platforms it will be difficult to change). I'd suggest this can be changed
really easily, by just using a member variable - add a member bool
m_fixedWidthImmedaites, initialize this to false, set it to true in the places
you set fixed = 1;, and check m_fixedWidthImmedaites instead of fixed in the
places you check the value.
> + Call tailRecursiveCall()
> + {
> + // Like a normal call, but don't link.
Whilst I understand this is completely correct, I think this comment has the
potential to be confusing, since in the context of calls we're usually use to
word 'link' to mean static link the JIT generated code. Being explicit and
saying something like "but don't set the link register" might just avoid any
confusion.
> Index: jit/ExecutableAllocator.h
> ===================================================================
> + int start = reinterpret_cast<intptr_t>(code) & (-line_size);
"int foo = reinterpret_cast<intptr_t>" should probably be "intptr_t foo =
reinterpret_cast<intptr_t>". :-)
I'll take another look over the whole patch when you're addressed these issues,
but nothing else stood out to me, all looks good otherwise.
cheers,
G.
--
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