[Webkit-unassigned] [Bug 169714] [jsc] Add missing MacroAssemblerMIPS::or32() implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 3 05:38:16 PDT 2017
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>
More information about the webkit-unassigned
mailing list