Closed Bug 875929 Opened 7 years ago Closed 7 years ago

x86/x64: optimize all floating-point constants which can be handled with pcmpeqw and shifts

Categories

(Core :: JavaScript Engine, enhancement)

x86_64
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch a proposed fixSplinter Review
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 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+
(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.
>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
Attached patch an additional patch (obsolete) — Splinter Review
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 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+
Attached patch refreshed patchSplinter Review
Refresh the patch to top of tree.
Attachment #755043 - Attachment is obsolete: true
Attachment #758341 - Flags: review?(evilpies)
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+
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
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.
Follow-up for the clobber issue:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb0927fe094

Bug 879809 filed for the build system issue.
You need to log in before you can comment on or make changes to this bug.