Bug 152865 – [mips] Add new or32 implementation after r194613

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Konstantin Tokarev]

Add new or32 implementation for MIPS after r194613

[Konstantin Tokarev]

Created attachment 268495 [details]
Patch

[Alex Christensen]

Comment on attachment 268495 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:384
> +        if (imm.m_value > 0 && imm.m_value < 65535

I'm no mips expert, but should this be <= 65535?  There are other places in MacroAssemblerMIPS.h where this is also < 65535, but don't they all work using the 16-bit immediate value if the immediate value is equal to 65535, too?

[Julien Brianceau]

Comment on attachment 268495 [details]
Patch

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

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:384
>> +        if (imm.m_value > 0 && imm.m_value < 65535

Please use a or32(TrustedImm32, RegisterID) call here to avoid code duplication, something like this:

        load32(dest.m_ptr, immTempRegister);
        or32(imm, dest);
        store32(immTempRegister, dest.m_ptr);


> I'm no mips expert, but should this be <= 65535?  There are other places in MacroAssemblerMIPS.h where this is also < 65535, but don't they all work using the 16-bit immediate value if the immediate value is equal to 65535, too?

You're right Alex, I also think this should be <= 0xffff. As there are other places in MacroAssemblerMIPS needing this change, maybe we could do this in a separate change ?

[Konstantin Tokarev]

Created attachment 268532 [details]
Patch

[Julien Brianceau]

Comment on attachment 268532 [details]
Patch

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

LGTM

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:381
> +        // TODO: Swap dataTempRegister and immTempRegister usage

Can you just add the (imm value == 0) check here to avoid load32/store32 in this case:

        if (!imm.m_value && !m_fixedWidth)
            return;

[Konstantin Tokarev]

Created attachment 268533 [details]
Patch

[Julien Brianceau]

LGTM, thanks

[Michael Saboff]

Comment on attachment 268533 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 268533 [details]
Patch

Clearing flags on attachment: 268533

Committed r194763: <http://trac.webkit.org/changeset/194763>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.