Closed
Bug 993034
Opened 10 years ago
Closed 10 years ago
SIMD: drive-by clean-ups
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bbouvier, Assigned: bbouvier)
Details
Attachments
(6 files)
5.10 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
27.24 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
8.93 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
9.46 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
8.09 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
While reading the SIMD code for understanding the API, I saw that the code could benefit from some clean ups. If you don't want to take all of them, that's fine; however I found them to make the code way more readable and logical.
Assignee | ||
Comment 1•10 years ago
|
||
This one renames toType2 into toType, and makes it fallible, as one variant of toType calls toInt32 which is fallible.
Attachment #8402783 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 2•10 years ago
|
||
This is the main patch. It tries to respect more SM's coding style, does some manual LICM at some places, replaces one series of instructions by a loop, deletes unnecessary parenthesis in conditions, takes advantage of local typedef to reduce code size, and so on.
Attachment #8402789 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 3•10 years ago
|
||
I don't get why this was working. orInt is an int32_t[] === int32_t*, so taking its address should return a int32_t**, which was then casted into float*, making types non consistent. This patch makes it cleaner and avoids the 3 unnecessary array slots.
Attachment #8402793 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 4•10 years ago
|
||
This one deletes the two gigantic LANE and SIGNMASK macros, replacing them with templated functions. We still need a LANE_ACCESSOR macro, because a macro apparently cannot take a templated method that has several template parameters (because of the comma).
Attachment #8402795 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 5•10 years ago
|
||
Having ObjectIsVector<V> is nice, but most callers are actually testing | value.isObject() && ObjectIsVector<V>(value) |, so let's just factor that too and have our conditions skinnier.
Attachment #8402797 -
Flags: review?(nmatsakis)
Assignee | ||
Comment 6•10 years ago
|
||
This one factors the reinterpret_cast(value.toObject().as<TypedObject>().typedMem()) out in a function, making all callers happier.
Attachment #8402799 -
Flags: review?(nmatsakis)
Updated•10 years ago
|
Attachment #8402783 -
Flags: review?(nmatsakis) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8402789 [details] [diff] [review] 2 - Cleaning up Review of attachment 8402789 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +542,2 @@ > CallArgs args = CallArgsFromVp(argc, vp); > + if (argc != 1 && argc != 2) { Nit: I'm told using argc is deprecated, and we ought to favor args.length()
Attachment #8402789 -
Flags: review?(nmatsakis) → review+
Updated•10 years ago
|
Attachment #8402793 -
Flags: review?(nmatsakis) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8402797 [details] [diff] [review] 5 - IsVectorObject Review of attachment 8402797 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +354,5 @@ > > RootedValue SIMDValue(cx, ObjectValue(*SIMD)); > > // Everything is set up, install SIMD on the global object. > + if (!JSObject::defineProperty(cx, global, cx->names().SIMD, SIMDValue, nullptr, nullptr, 0)) { nit: remove the `{...}` @@ +391,5 @@ > } > > template<typename V> > +static bool > +IsVectorObject(HandleValue v) Why not just merge these two functions together? Why keep the distinct `ObjectIsVector()`?
Attachment #8402797 -
Flags: review?(nmatsakis) → review+
Updated•10 years ago
|
Attachment #8402799 -
Flags: review?(nmatsakis) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8402795 [details] [diff] [review] 4 - Avoid Macros as much as possible Review of attachment 8402795 [details] [diff] [review]: ----------------------------------------------------------------- We actually had templates initially and moved to #define's -- I think the motivator was wanting to be able to have simple names we can reference from the MCallOptimize code. I too prefer editing `template<>` code to macros, though, so another option is to have macros declare a shallow wrapper that just calls the `template` function.
Attachment #8402795 -
Flags: review?(nmatsakis) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Thanks for the reviews! (In reply to Niko Matsakis [:nmatsakis] from comment #7) > Comment on attachment 8402789 [details] [diff] [review] > 2 - Cleaning up > Nit: I'm told using argc is deprecated, and we ought to favor args.length() Fixed. (In reply to Niko Matsakis [:nmatsakis] from comment #8) > Comment on attachment 8402797 [details] [diff] [review] > 5 - IsVectorObject > Why not just merge these two functions together? Why keep the distinct > `ObjectIsVector()`? Indeed, nice catch. (In reply to Niko Matsakis [:nmatsakis] from comment #9) > Comment on attachment 8402795 [details] [diff] [review] > 4 - Avoid Macros as much as possible > We actually had templates initially and moved to #define's -- I think the > motivator was wanting to be able to have simple names we can reference from > the MCallOptimize code. I too prefer editing `template<>` code to macros, > though, so another option is to have macros declare a shallow wrapper that > just calls the `template` function. Alright. I kept it as is. If we ever want simpler names again, it won't be too big a change. Running a try build to be sure (mixing templates and macros as in patch 4 could make some compilers angry) https://tbpl.mozilla.org/?tree=Try&rev=edf5a6d21a0c
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #10) > (mixing templates and macros as in patch 4 > could make some compilers angry) > https://tbpl.mozilla.org/?tree=Try&rev=edf5a6d21a0c Sometimes one doesn't like to be right. Reverting back to (more) shallow wrappers macros that call the templated method, as suggested in comment 9, is enough. https://tbpl.mozilla.org/?tree=Try&rev=1704da84aa3b (OS X reds are caused by bug 996621) I'll land it as soon as the OS X build returns green and the tree opens.
Assignee | ||
Comment 12•10 years ago
|
||
OK, infra red still present on B2G OS X Opt, but everywhere else is all green, so let's land it remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce48eb7d960 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/810593c39953 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b1f10cb214a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/87e9fc86154e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/330cc814b05b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c402f8b74fc
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6ce48eb7d960 https://hg.mozilla.org/mozilla-central/rev/810593c39953 https://hg.mozilla.org/mozilla-central/rev/6b1f10cb214a https://hg.mozilla.org/mozilla-central/rev/87e9fc86154e https://hg.mozilla.org/mozilla-central/rev/330cc814b05b https://hg.mozilla.org/mozilla-central/rev/9c402f8b74fc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•