Closed
Bug 875929
Opened 11 years ago
Closed 11 years ago
x86/x64: optimize all floating-point constants which can be handled with pcmpeqw and shifts
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: sunfish, Assigned: sunfish)
Details
Attachments
(2 files, 1 obsolete file)
5.44 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
8.66 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
MacroAssembler-x86-shared.h has optimized sequences for -0.0, 0.5, 2.0, and several other constants which can be emitted via pcmpeqw and shifts. However, it doens't cover all cases, including 0x7ff8000000000000, which is a fairly common NaN.
Assignee | ||
Comment 1•11 years ago
|
||
This patch generalizes the existing code. It required implementing js_bitscan_clz64 and js_bitscan_ctz64 on 32-bit platforms.
Attachment #753945 -
Flags: review?(evilpies)
Comment 2•11 years ago
|
||
Comment on attachment 753945 [details] [diff] [review] a proposed fix Review of attachment 753945 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/public/Utility.h @@ +214,3 @@ > > __forceinline static int > __BitScanForward64(unsigned __int64 val) Feels kind of wrong to move what seems to be the general implementation into something that was supposed to abstract msvc. @@ +246,4 @@ > } > # define js_bitscan_ctz64(val) __BitScanForward64(val) > # define js_bitscan_clz64(val) __BitScanReverse64(val) > # define JS_HAS_BUILTIN_BITSCAN64 I think this has some effect on jslog2.cpp. Not really builtin. Would you mind just cleaning up all of this mess? @@ +268,5 @@ > # define js_bitscan_clz32(val) __builtin_clz(val) > # define JS_HAS_BUILTIN_BITSCAN32 > + > +JS_STATIC_ASSERT(sizeof(unsigned long long) == sizeof(uint64_t)); > +# define js_bitscan_ctz64(val) __builtin_ctzll(val) So this is actually defined on x86?
Attachment #753945 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #2) > Comment on attachment 753945 [details] [diff] [review] > a proposed fix > > Review of attachment 753945 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice! > > ::: js/public/Utility.h > @@ +214,3 @@ > > > > __forceinline static int > > __BitScanForward64(unsigned __int64 val) > > Feels kind of wrong to move what seems to be the general implementation into > something that was supposed to abstract msvc. In my little world, __builtin_ctz etc. are the general implementation, so this doesn't feel as wrong :-). > > @@ +246,4 @@ > > } > > # define js_bitscan_ctz64(val) __BitScanForward64(val) > > # define js_bitscan_clz64(val) __BitScanReverse64(val) > > # define JS_HAS_BUILTIN_BITSCAN64 > > I think this has some effect on jslog2.cpp. Not really builtin. Would you > mind just cleaning up all of this mess? Ok, I'll see what I can do. Do you want me to update this patch, or submit a separate one? > @@ +268,5 @@ > > # define js_bitscan_clz32(val) __builtin_clz(val) > > # define JS_HAS_BUILTIN_BITSCAN32 > > + > > +JS_STATIC_ASSERT(sizeof(unsigned long long) == sizeof(uint64_t)); > > +# define js_bitscan_ctz64(val) __builtin_ctzll(val) > > So this is actually defined on x86? Yes.
Comment 4•11 years ago
|
||
>Ok, I'll see what I can do. Do you want me to update this patch, or submit a separate one?
Your decision. We can certainly do it an other bug and just take this.
Assignee: general → sunfish
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 5•11 years ago
|
||
Ok, here's an attempt at cleaning things up. This patch applies after the first one. Are any of the APIs involved here things that need to be preserved?
Attachment #755043 -
Flags: review?(evilpies)
Comment 6•11 years ago
|
||
Comment on attachment 755043 [details] [diff] [review] an additional patch Review of attachment 755043 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much, and sorry for the delay my computer broke. ::: js/src/moz.build @@ +155,5 @@ > 'jsfun.cpp', > 'jsgc.cpp', > 'jsinfer.cpp', > 'jsinterp.cpp', > 'jsiter.cpp', Yaay!
Attachment #755043 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Refresh the patch to top of tree.
Attachment #755043 -
Attachment is obsolete: true
Attachment #758341 -
Flags: review?(evilpies)
Comment 8•11 years ago
|
||
Comment on attachment 758341 [details] [diff] [review] refreshed patch rs=me. Doesn't look like that would have required a new review.
Attachment #758341 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Patches checked into mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2dadbc0908 https://hg.mozilla.org/integration/mozilla-inbound/rev/25ea966531ca
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla24
Comment 10•11 years ago
|
||
Failed to build initially, likely due to needing a clobber. Have clobbered + retriggered, if that is fine we'll need to update the CLOBBER file in the root of the repo, so the clobber propagates automatically around the various trees.
Comment 11•11 years ago
|
||
Follow-up for the clobber issue: https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb0927fe094 Bug 879809 filed for the build system issue.
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c2dadbc0908 https://hg.mozilla.org/mozilla-central/rev/25ea966531ca https://hg.mozilla.org/mozilla-central/rev/8bb0927fe094
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•