Bug 184584 - [MIPS] Optimize generated JIT code using mips32r2 instructions
Summary: [MIPS] Optimize generated JIT code using mips32r2 instructions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Other Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-13 04:49 PDT by Srdjan Lazarevic
Modified: 2018-05-30 08:56 PDT (History)
12 users (show)

See Also:


Attachments
Patch (32.08 KB, patch)
2018-04-13 05:10 PDT, Srdjan Lazarevic
ysuzuki: review-
Details | Formatted Diff | Diff
Patch (26.24 KB, patch)
2018-05-07 07:13 PDT, Srdjan Lazarevic
ysuzuki: review+
Details | Formatted Diff | Diff
Patch (23.87 KB, patch)
2018-05-09 05:58 PDT, Srdjan Lazarevic
srdjan.lazarevic: review?
ews: commit-queue-
Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2018-05-09 06:37 PDT, Srdjan Lazarevic
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.77 MB, application/zip)
2018-05-09 07:09 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Lazarevic 2018-04-13 04:49:46 PDT
EXT and MFHC1 instructions from MIPSR2 implemented and used where it is possible.
Also, done some code size optimizations that were discovered in meantime.
Comment 1 Srdjan Lazarevic 2018-04-13 05:10:28 PDT
Created attachment 337882 [details]
Patch
Comment 2 Build Bot 2018-04-13 05:12:22 PDT
Attachment 337882 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:241:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:242:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:243:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Srdjan Lazarevic 2018-04-18 05:27:41 PDT
Can anyone take a look at this change?
Comment 4 Mark Lam 2018-04-18 07:50:05 PDT
Comment on attachment 337882 [details]
Patch

I don't have much MIPS expertise but LGTM.  Someone who knows the MIPS instruction set should really review.

Also, would be better if you add some tests in testmasm.cpp that exercises your macro assembler functions.
Comment 5 Srdjan Lazarevic 2018-04-23 07:38:11 PDT
Yusuke Suzuki, are you OK with the change?
Comment 6 Srdjan Lazarevic 2018-05-03 08:57:15 PDT
Ping.
Comment 7 Yusuke Suzuki 2018-05-03 17:35:34 PDT
Comment on attachment 337882 [details]
Patch

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

The logic of the code looks good, but there are so much duplicate code. Can we clean up them?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2933
> +            if (!address.scale)
> +                m_assembler.addu(addrTempRegister, address.index, address.base);
> +            else {
> +                m_assembler.sll(addrTempRegister, address.index, address.scale);
> +                m_assembler.addu(addrTempRegister, addrTempRegister, address.base);
> +            }
> +            if (address.offset < -32768 || address.offset > 32767) {
> +                m_assembler.lui(immTempRegister, (address.offset + 0x8000) >> 16);
> +                m_assembler.addu(addrTempRegister, addrTempRegister, immTempRegister);
> +            }

This code pattern is so repeated. Can we extract this pattern to remove duplicate code?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2934
> +                m_assembler.swc1(src, addrTempRegister, address.offset);

Fix this indent.
Comment 8 Srdjan Lazarevic 2018-05-07 07:13:28 PDT
Created attachment 339717 [details]
Patch

Clean up duplicate code.
Comment 9 Build Bot 2018-05-07 07:16:07 PDT
Attachment 339717 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:241:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:242:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:243:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Yusuke Suzuki 2018-05-07 18:20:31 PDT
Comment on attachment 339717 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:323
> +    void loadAddress(BaseIndex address, bool loadTrail = true)

Use `enum class` instead of `bool`. In this case, not using default parameter is better.
And `loadTrail` is a bit confusing. Maybe, `enum class LoadAddressMode { DontAddOffset, AddOffset };` would be more descriptive.
Comment 11 Srdjan Lazarevic 2018-05-09 05:58:54 PDT
Created attachment 339958 [details]
Patch

'Enum class' instead 'bool', removed default parameter.
Comment 12 Build Bot 2018-05-09 06:00:38 PDT
Attachment 339958 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:241:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:242:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:243:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Yusuke Suzuki 2018-05-09 06:05:00 PDT
Comment on attachment 339958 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:117
> +        ScaleAndAddOffset,

I think this enum name is a bit misleading since `offset` is only added if `offset` is large / small enough.
Comment 14 Yusuke Suzuki 2018-05-09 06:05:53 PDT
(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 339958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339958&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:117
> > +        ScaleAndAddOffset,
> 
> I think this enum name is a bit misleading since `offset` is only added if
> `offset` is large / small enough.

Maybe the other enum name would be fine. Like, `ScaleAndAddOffsetIfOffsetIsOutOfBounds`.
Comment 15 Srdjan Lazarevic 2018-05-09 06:37:12 PDT
Created attachment 339960 [details]
Patch

Fixed. :)
Comment 16 Build Bot 2018-05-09 06:39:11 PDT
Attachment 339960 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:241:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:242:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:243:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2018-05-09 07:09:43 PDT
Comment on attachment 339958 [details]
Patch

Attachment 339958 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7623156

New failing tests:
http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients.html
Comment 18 Build Bot 2018-05-09 07:09:44 PDT
Created attachment 339962 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 19 WebKit Commit Bot 2018-05-09 07:17:06 PDT
Comment on attachment 339960 [details]
Patch

Clearing flags on attachment: 339960

Committed r231554: <https://trac.webkit.org/changeset/231554>
Comment 20 Yusuke Suzuki 2018-05-30 08:55:41 PDT
Seems landed.
Comment 21 Radar WebKit Bug Importer 2018-05-30 08:56:36 PDT
<rdar://problem/40651435>