WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
184584
[MIPS] Optimize generated JIT code using mips32r2 instructions
https://bugs.webkit.org/show_bug.cgi?id=184584
Summary
[MIPS] Optimize generated JIT code using mips32r2 instructions
Srdjan Lazarevic
Reported
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.
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
ews-watchlist
: 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
,
EWS Watchlist
no flags
Details
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Srdjan Lazarevic
Comment 1
2018-04-13 05:10:28 PDT
Created
attachment 337882
[details]
Patch
EWS Watchlist
Comment 2
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.
Srdjan Lazarevic
Comment 3
2018-04-18 05:27:41 PDT
Can anyone take a look at this change?
Mark Lam
Comment 4
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.
Srdjan Lazarevic
Comment 5
2018-04-23 07:38:11 PDT
Yusuke Suzuki, are you OK with the change?
Srdjan Lazarevic
Comment 6
2018-05-03 08:57:15 PDT
Ping.
Yusuke Suzuki
Comment 7
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.
Srdjan Lazarevic
Comment 8
2018-05-07 07:13:28 PDT
Created
attachment 339717
[details]
Patch Clean up duplicate code.
EWS Watchlist
Comment 9
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.
Yusuke Suzuki
Comment 10
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.
Srdjan Lazarevic
Comment 11
2018-05-09 05:58:54 PDT
Created
attachment 339958
[details]
Patch 'Enum class' instead 'bool', removed default parameter.
EWS Watchlist
Comment 12
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.
Yusuke Suzuki
Comment 13
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.
Yusuke Suzuki
Comment 14
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`.
Srdjan Lazarevic
Comment 15
2018-05-09 06:37:12 PDT
Created
attachment 339960
[details]
Patch Fixed. :)
EWS Watchlist
Comment 16
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.
EWS Watchlist
Comment 17
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
EWS Watchlist
Comment 18
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
WebKit Commit Bot
Comment 19
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
>
Yusuke Suzuki
Comment 20
2018-05-30 08:55:41 PDT
Seems landed.
Radar WebKit Bug Importer
Comment 21
2018-05-30 08:56:36 PDT
<
rdar://problem/40651435
>
Guillaume Emont
Comment 22
2020-06-04 04:01:02 PDT
Comment on
attachment 339958
[details]
Patch Clearing r? flag as a variant of the patch landed in
r231554
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug