725198 - js_DoubleToECMAUint32 implementation seems to be unnecessary

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

As far as I can tell, simply returning js_DoubleToEcmaInt32 cast to uint32_t would do the right thing.  And the conversion to Int32 is all hand-optimized and stuff, so it would be faster.  Is there a reason we're not doing that?

Comment 1

[Andreas Gal :gal]

If I remember correctly js_DoubleToEcma(U)Int32 is equivalent to unsigned(d) and signed(d) as long its not a corner case (finite value and such).

  double d = 0xffffffffUL;
  unsigned u = unsigned(d);
  signed i = signed(d);
  printf("%u %d %u\n", u, i, unsigned(i));

prints

4294967295 -2147483648 2147483648

So I don't think this works, but it has been a long day and I am jetlagged to pieces, so if I am on crack, I apologize in advance :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

js_DoubleToEcmaInt32(double(0xffffffffUL)) would do the following:

1)  Floor the number (it's already an integer, so a no-op).
2)  Reduce mod 2^32 (already in the right range, so a no-op).
3)  Subtract 2^32, since it's >= 2^31

Hence it returns -1, unless I'm missing something.  And in fact, in JS (0xffffffff >> 0) is -1.

signed(d) for that double is not actually defined in C/C++, so the compiler can kinda make up whatever it wants.  But yours is definitely not making up -1.  ;)

Comment 3

[David Mandelin [:dmandelin]]

(In reply to Boris Zbarsky (:bz) from comment #0)
> As far as I can tell, simply returning js_DoubleToEcmaInt32 cast to uint32_t
> would do the right thing.  

The spec agrees with you: 9.5 says that

  for all x: ToUint32(ToInt32(x)) == ToUint32(x)

> And the conversion to Int32 is all hand-optimized
> and stuff, so it would be faster.  Is there a reason we're not doing that?

No idea. I think older editions of the spec used to be a little less clear about this, so maybe it was different, or maybe it just wasn't as clear that it works.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 595595 [details] [diff] [review]
Patch

>+inline uint32_t
>+js_DoubleToECMAUint32(jsdouble d) {

{ on new line

Comment 6

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1b0466feed19

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b0466feed19