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

[bugzilla-daemon at webkit.org]

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





--- Comment #93 from Gavin Barraclough <barraclough at apple.com>  2009-08-05 16:18:18 PDT ---
(From update of attachment 34129)
(1) ensureSpaceFor

The basic design of this I'm happy with, the idea of enumerating the sequences
seems nice and clean, but I do have two small concerns.  First is the method
name - I think it could be clearer, and more explicit.  The method name makes
sense in the context of ARM, where you have a constant pool, but for developers
on other platforms it is unclear as to the purpose of the call.I'd suggest a
better name would include something like UninterruptedInstructionSequence,
which would be more descriptive of *why* this is being called.   Also, I'm
uncomfortable with the use of magic constants that are unchecked (note that all
the 'patchOffset' constants in the JIT are guarded by ASSERTs).  If someone
changes the code generation in a way that could break on ARM, why should have
an automatic way of detecting this.  I'd suggest that you can fix this by
having 'begin' and 'end' methods in the MacroAssemberARM, and #ifndef NDEBUG
check that the values passed are correct.

I'd suggest something like this (with
MacroAssemblerARM::beginUninterruptedInstructionSequence replacing
MacroAssemblerARM::ensureSpace, and
MacroAssemblerARM::endUninterruptedInstructionSequence called at the end of the
sequence as appropriate - both method names just suggestions, I'm open to
improvements), would both be more explicitly named, and would provide checking
of at least one of the magic constants:

    void beginUninterruptedInstructionSequence(int insnSpace, int constSpace)
    {
#ifndef NDEBUG
        m_uninterruptedInstructionsBegin = label();
        m_uninterruptedInstructionSpace = insnSpace;
#endif
        m_assembler.ensureSpace(insnSpace, constSpace);
    }

    void endUninterruptedInstructionSequence()
    {
        ASSERT(differenceBetween(m_uninterruptedInstructionsBegin, label()) ==
m_uninterruptedInstructionSpace);
    }

#ifndef NDEBUG
    Label m_uninterruptedInstructionsBegin;
    int m_uninterruptedInstructionSpace;
#endif

If you could also preserve and test 'constSpace' in a similar fashion (could
you record the number of entries in the pool in 'begin', and again check the
difference in 'end'?), then that would be even better.

(2) enableLatePatch

This seems to be tied to an assembler specific internal optimization, I don't
think that explicitly exposing this in common code in this fashion is the right
API here.  The purpose of this flag appears to be to force calls to be
generated using a constant pool entry, rather than as a relative branch.  A
simple short-term solution would seem to replace the 'm_latePatch' bit with a
flag indicating whether the JmpSrc is a jump or a call, and to only plant
relative branches for jumps.  With this change the calls to enableLatePatch()
should be unnecessary - all calls can be repatched.

(As a note on a longer term direction, we expect at some point to be able to
move the Label, Jump, Call, etc classes out of AbstractMacroAssembler and into
their own header, then push these types down into the assemblers so that each
assembler does not need define its own JmpSrc & JmpDst types - so in the long
run I think we'd expect to be able to distinguish between jumps & calls
anyway).

This suggested change is based on the assumption that the ability to link
branches without a constant pool entry is an optimization that only really
benefits jumps, not calls.  I'd be resistant to introduce any additional API
unless there is a demonstrable benefit here.  If there is a measurable
performance benefit from doing so, then I'd suggest the right place to add API
would be in LinkBuffer::link(Call, FunctionPtr).  Ultimately this is not about
the call instruction itself being different, but the way that the link will be
performed being different.  I'd suggest adding a third argument to this method
- ", bool isFinal = false" (I've inverted the logic from 'enable' here - I'd
suggest the default behaviour should be to allow the call to be relinked, and
if we do need to do so we should explicitly specify cases where optimization
can take place).  For calls that are *not* currently being marked as
'enableLatePatch', you could instead pass isFinal as true when linking, which
should result in exactly the same code generation from the ARM JIT as you are
presently producing.  But I'd like to see performance numbers to back up any
API change, since I do expect that the benefit is really coming from planting
relative branches for jumps rather than calls.

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