[webkit-reviews] review denied: [Bug 31060] Use ARMv7 specific encoding for immediate constants on ARMv7 target : [Attachment 42381] Use ARMv7 specific encoding for immediate constants on ARMv7 target

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at inf.u-szeged.hu>'s request for review:
Bug 31060: Use ARMv7 specific encoding for immediate constants on ARMv7 target
https://bugs.webkit.org/show_bug.cgi?id=31060

Attachment 42381: Use ARMv7 specific encoding for immediate constants on ARMv7
target
https://bugs.webkit.org/attachment.cgi?id=42381&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
Having 0 as the "cannot encode" value for getImm16Op2 is probably not a good
idea, since 0 is a common immediate, and could be encoded in one instruction.

I guess this is not actively a problem in this patch, since you don't call
encodeComplexImm on zero – but to guard against this mistake creeping in you
should probably ASSERT(imm != 0) in this function, or use a value other than 0
to mean 'cannot encode'.

We also prefer to avoid magic values in the code, so I'd suggest that you
should define up a name for this (e.g. something like static const ARMWord
INVALID_IMM16 0;), then return 0; should be return INVALID_IMM16;, if (tmp)
should be if (tmp != INVALID_IMM16).

It would also probably be a good idea to add guards to movw_r & movt_r that op2
is valid.

r-, because I think some extra ASSERTs here are worthwhile, but the patch is
otherwise all good.