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

[bugzilla-daemon at webkit.org]

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





--- Comment #83 from Gavin Barraclough <barraclough at apple.com>  2009-07-29 21:01:45 PDT ---
(From update of attachment 33609)
(1) The CodeBlock changes.

Having divergent behaviour outside of the JIT is really undesirable – while
these differences look fairly harmless right now, as we changes the way the JIT
works over time, they could become problematic.  This approch also seems
suboptimal on ARM platforms, since ARM is typically used in embedded products
with limited memory availability, and carrying around all bytecode from all
functions is a large memory burden (particularly on complex sites using large
JS libraries).  I'd suggest that there is a much better solution, that isolates
the issue to the MacroAssembler, and will overall significantly reduce the
memory footprint of the JIT on ARM.

I'm assuming that you're running into a problem that we also ran into on ARMv7,
that sometimes a pointer value will be representable as an instruction
immediate?  On ARMv7, when a ImmPtr is converted to an Imm32 we set a flag in
the Imm32 indicating that it is wrapping a pointer value - Imm32::m_isPointer. 
You could just change the #if wrapping this to PLATFORM(ARM).  In fact, having
one extra field passed in Imm32 is highly unlikely to seriously impact JIT
performance (most methods using this are inline to the extra value should just
fold away in the compiler), so with a quick performance check you could
probably just remove the #if altogether.

In MacroAssemblerARMv7, when emitting an instruction with an Imm32 operand, we
force all immediate operands for which this flag is set to be generated in a
manner that could represent a full 32-bit value (i.e., in your case, that would
mean generating the value into the constant pool).  Whilst this clearly this
means occasionally adopting a slightly less optimal representation for a few
pointer values scattered in the code, when balanced against the memory saving
of not retaining the bytecode it seems to be a clear huge win.


(2) JIT generating the trampolines.

Again, I'm really not keen on this, specifically because it differs from the
way other platforms work, and having randomly differing implementations makes
the JIT unnecessarily complex for a new developer to start working on (and JITs
tend not to be the simplest pieces of code under the best of circumstances).  I
actually much prefer your way of doing things here - I'd much prefer if we
could JIT generate these trampolines on all platforms.  However we tested this
on x86, and when we did we measured a repeatable performance regression (I'm
presuming this is because it changes a well predicted relative call instruction
to a memory-indirected poorly predicted call).  So my question would be, is
there any significant benefit to ARM from this?  If there is not a clear
benefit from doing so, then introducing unnecessary inconsistencies would be
highly undesirable.


> +#if PLATFORM(ARM) && !PLATFORM_ARM_ARCH(7)
> +    Jump jumpToCtiVMThrowTrampoline = jump();
> +#else
> +    move(ImmPtr(reinterpret_cast<void*>(ctiVMThrowTrampoline)), regT2);
>      restoreReturnAddressBeforeReturn(regT2);
>      ret();
> -    
> +#endif

The JIT is currently making a jump to the ctiVMThrowTrampoline in a really
weird way here, but it doesn't seem like a great idea to only fix this on ARM –
and it having this code unnecessarily platform specific would be undesirable. 
We really should just switch this to a jump on all platforms. 


>              return JSValue::decode(ctiTrampoline(
>  #if PLATFORM(X86_64)
>                  0, 0, 0, 0, 0, 0,
> +#elif PLATFORM(ARM)
> +                // For both ARM and ARMv7
> +                0, 0, 0, 0,
>  #endif
>                  m_ref.m_code.executableAddress(), registerFile, callFrame, exception, Profiler::enabledProfilerReference(), globalData));

It looks from the comment like this was a deliberate change to ARMv7 – if so
(and I apologize if I'm misinterpreting here), please don't make speculative
changes to platforms you aren't testing (a bug report would of course be hugely
appreciated if you think you spot a bug), this 'fix' will actually break ARMv7.
 On ARMv7 the trampoline accepts arguments in registers, spilling those that
need be preserved to the stack.  This both seems somewhat cleaner (rather than
relying on the 'magic' behavior of passing extra arguments), and also allows us
to chose not to spill those arguments that we do not need to preserve (e.g. the
entry code pointer).  We have a long standing bug open to change x64-64 to work
this way too, and I'd suggest you may want to consider the same for ARM -
though that is purely optional, just switching to 'PLATFORM(ARM) &&
!PLATFORM_ARM_ARCH(7)' would be a valid fix here.


> +#if PLATFORM(ARM)
> +    typedef EncodedJSValue (*ctiTrampolineFunctionPtr)(void*, void*, void*, void*,
> +            void* code, RegisterFile*, CallFrame*, JSValue* exception, Profiler**, JSGlobalData*);
> +    extern ctiTrampolineFunctionPtr ctiTrampoline;
> +    extern void* ctiVMThrowTrampoline;
> +    extern void* ctiOpThrowNotCaught;
> +#else

This should be 'PLATFORM(ARM) && !PLATFORM_ARM_ARCH(7)'


> -#if PLATFORM(ARM)
> +#if PLATFORM_ARM_ARCH(7)
>  #define ENABLE_ASSEMBLER_WX_EXCLUSIVE 1
>  #else
>  #define ENABLE_ASSEMBLER_WX_EXCLUSIVE 0

This was probably really on the wrong #ifdef already, so if we're going to
change it, then it should probably really go to #if PLATFORM(IPHONE).

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