Bug 169714 – [jsc] Add missing MacroAssemblerMIPS::or32() implementation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Guillaume Emont]

We miss MacroAssemblerMIPS::or32(TrustedImm32, Address).

[Guillaume Emont]

Created attachment 304573 [details]
Patch

The patch.

[Adrian Perez]

Comment on attachment 304573 [details]
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

[Guillaume Emont]

(In reply to Adrian Perez from comment #2)
> Comment on attachment 304573 [details]
> 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 && ...)
> 
Indeed, this would be clearer, especially with the bitmask I'd think. I'll create a separate bug and try to address that for the whole file.


> > 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.
I am cooking a patch that address this issues. Here again, I just copy-pasted from another version of the function, and that issue is a bit everywhere in the file and deserves its own bug I guess.

> 
> > 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

[Guillaume Emont]

Comment on attachment 304573 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:481
> +                m_assembler.addu(dataTempRegister, dataTempRegister, immTempRegister);

Wow! I did not pay attention when doing that. This obviously should be an orInsn not an addu! Will also fix in the new version. The fact that this mistake was not obvious when looking at test results makes me think that this code path (or the whole function) might never be used.

[Guillaume Emont]

Created attachment 306436 [details]
Patch

Patch with comments addressed. I am currently testing it to check it does not break anything (tests take about 10h to run, and bot is testing another revision before).

[Guillaume Emont]

(In reply to Guillaume Emont from comment #5)
> Created attachment 306436 [details]
> Patch
> 
> Patch with comments addressed. I am currently testing it to check it does
> not break anything (tests take about 10h to run, and bot is testing another
> revision before).

And the test ran: this patch did not introduce any new crash, so I believe it's good to go.

[Adrian Perez]

\o/(In reply to Guillaume Emont from comment #6)
> (In reply to Guillaume Emont from comment #5)
> > Created attachment 306436 [details]
> > Patch
> > 
> > Patch with comments addressed. I am currently testing it to check it does
> > not break anything (tests take about 10h to run, and bot is testing another
> > revision before).
> 
> And the test ran: this patch did not introduce any new crash, so I believe
> it's good to go.

\o/

Informal r+/cq+ from me.

[Michael Catanzaro]

Comment on attachment 306436 [details]
Patch

This is a rubber stamp since I don't understand any of it, but if Adrian says it's OK then let's do it.

[WebKit Commit Bot]

Comment on attachment 306436 [details]
Patch

Clearing flags on attachment: 306436

Committed r215237: <http://trac.webkit.org/changeset/215237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.