[Webkit-unassigned] [Bug 24986] ARM JIT port

Fri Jun 12 20:59:52 PDT 2009

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

------- Comment #26 from barraclough at apple.com  2009-06-12 20:59 PDT -------
We really like to try to keep the sizes of individual changes down, and this
patch is really doing two separate things – enabling ARM JIT support on YARR,
and enabling the JS JIT on ARM (in the case of the latter we have also landed
this in stages - first landing the JIT with optimizations disabled, and then
separate patches to enable the optimizations).  We would much prefer to land
these changes separately, since it both makes it much easier to properly review
the code, and more importantly if we ever discover a bug or performance problem
has been introduced, then it is a nightmare if we end up tracking it back to a
huge changeset.

So I'd really like to see a first patch that just enables YARR_JIT on ARM – we
can follow up immediately after with the rest of the JS changes.  This means
basically reverting everything in the JIT, interpreter, bytecode directories
for now, and raising a second patch as soon as this has landed.

Looking at the changes to the AssemblerBuffer changes, I think the
restructuring here isn't really working yet – which I think is somewhat
reflected by the fact that you (presumably) needed to have a mix of
ARMAssembler and AssemblerBufferARM methods within AssemblerBufferARM.cpp (and
keeping the functions that need inspect the generated code closely tied in with
the assembler does make sense to me).  We will want a constant pool available
on all platforms, but we will want a single, generic implementation of the core
infrastructure, shared on all architectures.  I'm not going to suggest that you
need to refactor that out before you can land this patch since that may be a
significant task, but we will need to do this soon since other platforms will
likely need constant pools, and we don't want duplicated implementations.

I'd suggest that the best thing to do right now to prevent forking the
AssemblerBuffer (which is not ultimately what we want to do) without causing
you too much work, would be to simply look at temporarily moving the
constant-pool related functionality back up into ARMAssembler (perhaps move the
new fields added to AssemblerBufferARM into a new ARMConstantPool class for
now, rather than adding the functionality into the existing class).  We can
then as a separate change work out how we design a common constant pool
mechanism across all platforms without having to hold this patch up.  What are
your thoughts?

Also in the AssemblerBuffer, we really want to keep the cache flushing code all
in one place, which is in ExecutableAllocator::cacheFlush.  We also need the
repatch methods to use the ExecutableAllocator::makeWritable /
ExecutableAllocator::MakeExecutable / the ExecutableAllocator::MakeWritable
class for cache-flushing new code and repatching, as the other assemblers do. 
This will mean that the ARM JIT will work with the ASSEMBLER_WX_EXCLUSIVE
mechanism on platforms where this is required or desired for security reasons. 
Also we intend to modify the way in which we repatch code so that we can
coalesce protection changes and cache flushes, so it is important that the
assemblers all use the same interfaces so we can keep the changes in sync.

The #defines are also going to need a little cleaning up.  We shouldn't be
defining macros beginning with __ - we treat these as being reserved for
compilers to define.  You could just rename __ARM_ARCH__ to ARM_ARCH_VERSION to
fix this, though I'd suggest if could be nice to define a macro like
HAS_ARM_ARCH(N) to perform the check too.  PLATFORM(ARM) is also defined on
ARM-v7 builds, so any code specific to the v6 & earlier ARM JIT will need to
check something like (PLATFORM(ARM) && ! PLATFORM(ARM_V7)) or (PLATFORM(ARM) &&
(ARM_ARCH_VERSION < 7)), or (PLATFORM(ARM) && !HAS_ARM_ARCH(7)) – or maybe have
a new PLATFORM(ARM_PRE_v7) defined in Platform.h.  I'm open to suggestions
here.

I'm a little concerned at having bkpt be a nop on pre-ARM-v5 platforms.  If the
instruction doesn't exist, I think you really should remove it from the
ARMAssembler interface (exactly as you have for clz_r).  I'd suggest that on
pre-v5 ARMs we really want to do something to stop execution from simply
ploughing on through breakpoint()s.  A simple suggestion would be to implement
MacroAssemblerARM::breakpoint() to plant a store to address 0 (I don't know ARM
memory maps, but most platforms will leave this page unmapped to catch errors).
 Alternatively you could plant a supervisor mode instruction which should cause
a trap, or just infinite loop.

You may want to think about modifying the behavior of MacroAssemblerARM::ret().
 You've adhered to the same behavior as x86, but I'm afraid we haven't been as
well behaved on ARMv7, and you may want to consider copying.  It is clearly not
ideal to have the behavior of MacroAssemblerARM::ret() differ between
platforms, but it is also clearly not ideal to require all other platforms to
mimic x86 behavior, and to push the return value on the stack.  We've certainly
not come up with the best and final solution here yet – the best solution may
be to say that function entry and exit simply can't be described well by the
MacroAssembler, and to remove ret() from the intergface & call
m_assembler.ret() directly.  Another solution for some cases may be to provide
a higher level interface to describe function entry and exit to the
MacroAssembler.

Finally there are a bunch of little code-style issues.  We do stick to full
names for variable naming, so cpool should be constantPool, CPool should be
ConstantPool.  We prefer inline methods to macro where possible, so
ARM_COND_FIELD should be an inline method armConditionField.  In 'emitInst'
there is an extra space between the ASSERT and open paren.  In Platform.h there
should not be a space between the # and the define/undef.  We don't prefix
constants with a 'k' – I'm not sure we have an entirely consistent style on
constants, but for values like this I'd say we tend to stick to
CAPS_AND_UNDERBARS (weinig/bdash/darin, I don't know if you want to chip in
here, you normally point out my style errors :-) ).

I'm going to r- for now, but don't worry - this is not as harsh as it sounds.
This is a perfectly normal stage in the inclusion of a set of changes of this
size.

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