[Webkit-unassigned] [Bug 49432] Support JIT_OPTIMIZE_MOD on Thumb-2

[bugzilla-daemon at webkit.org]

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


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73721|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Gavin Barraclough <barraclough at apple.com>  2010-11-19 20:45:16 PST ---
(From update of attachment 73721)
View in context: https://bugs.webkit.org/attachment.cgi?id=73721&action=review

Hey Loki,

Looks like a great patch, a couple of small issues.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:1719
> +#if CPU(ARM_THUMB2)
> +    RegisterID regS0 = ARMRegisters::r6;
> +    RegisterID regS1 = ARMRegisters::r7;
> +#endif

I'm not sure about defining these register names up here.  Currently all registers used by the JIT are effectively documented in JSInterfaceJIT.h, if someone wants to use a register they have one place to go look to see if it is used by the JIT, needs preserving in the entry trampolines, etc.  My concern is that a developer won't know to come look here to discover these registers are used.  I think it would be better to add definitions for regT5 & regT6 in JSInterfaceJIT.h, for ARM & ARMv7.

> JavaScriptCore/assembler/MacroAssemblerARMv7.h:226
> +    void clz(RegisterID src, RegisterID dest)
> +    {
> +        m_assembler.clz(dest, src);
> +    }

I'm not sure about this method.  The first question is should this be in the MacroAssembler interface at all? - do we believe it can be usefully abstracted across a range of platforms? – x86's BSR has somewhat different semantics, so I'm not sure we'll want to generally use this call in a cross platform way - but maybe we'd abstract like this (I find BSR a little weird to use), so I'm inclined to lean towards yes, keep this here (the alternative being to just call to m_assembler.clz directly).  But if we keep this, the name should change.  At minimum the operation size should be encoded in the name - clz32() (the name clz is correct at the assembler layer, this is just for the MacroAssemlber layer).  But coding style ask us to steer clear on acronyms & abbreviations (unless they're particularly standard and conventional), and I think we should probably change clz32 to something more explicit - like countLeadingZeros32().  What do you think?

r+ with these changes, r-ing this patch.

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.