Bug 152865 - [mips] Add new or32 implementation after r194613
Summary: [mips] Add new or32 implementation after r194613
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-07 14:40 PST by Konstantin Tokarev
Modified: 2016-01-08 08:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.89 KB, patch)
2016-01-07 14:42 PST, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2016-01-08 03:20 PST, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2016-01-08 03:31 PST, Konstantin Tokarev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Tokarev 2016-01-07 14:40:58 PST
Add new or32 implementation for MIPS after r194613
Comment 1 Konstantin Tokarev 2016-01-07 14:42:26 PST
Created attachment 268495 [details]
Patch
Comment 2 Alex Christensen 2016-01-07 17:44:06 PST
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?
Comment 3 Julien Brianceau 2016-01-08 01:04:44 PST
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 ?
Comment 4 Konstantin Tokarev 2016-01-08 03:20:47 PST
Created attachment 268532 [details]
Patch
Comment 5 Julien Brianceau 2016-01-08 03:27:04 PST
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;
Comment 6 Konstantin Tokarev 2016-01-08 03:31:03 PST
Created attachment 268533 [details]
Patch
Comment 7 Julien Brianceau 2016-01-08 04:27:56 PST
LGTM, thanks
Comment 8 Michael Saboff 2016-01-08 07:42:59 PST
Comment on attachment 268533 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2016-01-08 08:29:02 PST
Comment on attachment 268533 [details]
Patch

Clearing flags on attachment: 268533

Committed r194763: <http://trac.webkit.org/changeset/194763>
Comment 10 WebKit Commit Bot 2016-01-08 08:29:07 PST
All reviewed patches have been landed.  Closing bug.