Bug 156260 - MIPS: support Signed cond in branchTest32()
Summary: MIPS: support Signed cond in branchTest32()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-05 16:37 PDT by Guillaume Emont
Modified: 2016-04-08 18:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.97 KB, patch)
2016-04-05 16:42 PDT, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (2.97 KB, patch)
2016-04-05 16:45 PDT, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2016-04-06 09:45 PDT, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guillaume Emont 2016-04-05 16:37:11 PDT
This is needed since r197688 makes use of it.
Comment 1 Guillaume Emont 2016-04-05 16:42:36 PDT
Created attachment 275714 [details]
Patch

A proposed patch addressing the issue
Comment 2 WebKit Commit Bot 2016-04-05 16:43:42 PDT
Attachment 275714 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1521:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Guillaume Emont 2016-04-05 16:45:28 PDT
Created attachment 275716 [details]
Patch

A proposed patch addressing the issue
Comment 4 Julien Brianceau 2016-04-06 00:49:15 PDT
Comment on attachment 275716 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1505
> +        if ((cond != Signed) && m_fixedWidth)
> +            m_assembler.nop();

To me, these 2 lines are not needed.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1515
> +            ASSERT_NOT_REACHED();

Very good idea to add this, but please use "RELEASE_ASSERT_NOT_REACHED" instead.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1532
> +                ASSERT_NOT_REACHED();

Ditto.
Comment 5 Guillaume Emont 2016-04-06 09:45:35 PDT
Created attachment 275777 [details]
Patch

New version addressing Julien's remarks. Thanks for the quick review!
Comment 6 Julien Brianceau 2016-04-06 09:49:17 PDT
Looks good to me :)
Comment 7 Mark Lam 2016-04-06 09:53:49 PDT
Comment on attachment 275777 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2016-04-08 18:20:28 PDT
Comment on attachment 275777 [details]
Patch

Clearing flags on attachment: 275777

Committed r199261: <http://trac.webkit.org/changeset/199261>
Comment 9 WebKit Commit Bot 2016-04-08 18:20:32 PDT
All reviewed patches have been landed.  Closing bug.