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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

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.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

This patch generalizes the existing code. It required implementing js_bitscan_clz64 and js_bitscan_ctz64 on 32-bit platforms.

Comment 2

[Tom S. (please needinfo tschuster)]

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?

Comment 3

[Dan Gohman [:sunfish]]

(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

[Tom S. (please needinfo tschuster)]

>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.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: an additional patch

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?

Comment 6

[Tom S. (please needinfo tschuster)]

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!

Comment 7

[Dan Gohman [:sunfish]]

Attachment: refreshed patch

Refresh the patch to top of tree.

Comment 8

[Tom S. (please needinfo tschuster)]

Comment on attachment 758341 [details] [diff] [review]
refreshed patch

rs=me. Doesn't look like that would have required a new review.

Comment 9

[Dan Gohman [:sunfish]]

Patches checked into mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2dadbc0908
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ea966531ca

Comment 10

[Ed Morley [:emorley]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c2dadbc0908
https://hg.mozilla.org/mozilla-central/rev/25ea966531ca
https://hg.mozilla.org/mozilla-central/rev/8bb0927fe094