[webkit-reviews] review denied: [Bug 49432] Support JIT_OPTIMIZE_MOD on Thumb-2 : [Attachment 73721] Support JIT_OPTIMIZE_MOD on Thumb-2

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at webkit.org>'s request for review:
Bug 49432: Support JIT_OPTIMIZE_MOD on Thumb-2
https://bugs.webkit.org/show_bug.cgi?id=49432

Attachment 73721: Support JIT_OPTIMIZE_MOD on Thumb-2
https://bugs.webkit.org/attachment.cgi?id=73721&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.