[Webkit-unassigned] [Bug 169714] [jsc] Add missing MacroAssemblerMIPS::or32() implementation

[bugzilla-daemon at webkit.org]

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

--- Comment #2 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 304573
  --> https://bugs.webkit.org/attachment.cgi?id=304573
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304573&action=review

Reviewing informally: The comparisons should treat immediate value
zero as a 16-bit value which gets embedded in an “ori” instruction.
Other than that looks good overall.

There is the thing that I would prefer using constants from “stdint.h” or
masking with 0xFFFF to check for values outside of the 16-bit range, but
the rest of the code in the file compares directly against the numeric
values, so I don't have a strong feeling about that. It might be a good
idea to do change the code to use [U]INTx_{MIN,MAX} or bitmaks, as a
separate patch. WDYT?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:448
> +        if (address.offset >= -32768 && address.offset <= 32767

Personally, I would write this condition as...

      if (address.offset & 0xFFFF == 0 && ...)

...because when reading this form it makes me think “this checks whether
the upper 16-bits are unset”. On the other hand, comparisons against the
integral values keep me wondering “wait, why is there a magic number here?”.

I have no strong opinion about whether to use the bitmask version or the
integer comparison, but if you would rather keep the integer comparison,
please use INT16_MIN/INT16_MAX, which makes it more explicit that values
outside the 16-bit integer range are being handled:

       if (address.offset >= INT16_MIN && address.offset <= INT16_MAX && ...)

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:457
> +            if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth)

This should be “greater or equal to zero”, not just ”greater than zero”:

     if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...)
  or if (imm.m_value & 0xFFFF == 0 ...)

If the immediate value is equal to zero, emitting the code for ORing
could even be skipped altogether. Though I would do that separately
on another patch.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:477
> +            if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth)

Ditto: if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...)
                       ^^
                 greater or equal

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170403/bc8105ca/attachment-0001.html>