js_DoubleToECMAUint32 implementation seems to be unnecessary

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: dmandelin)

Tracking

Trunk
mozilla13
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

6 years ago
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 :)
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.  ;)
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 595595 [details] [diff] [review]
Patch
Assignee: general → dmandelin
Attachment #595595 - Flags: review?(luke)

Comment 5

5 years ago
Comment on attachment 595595 [details] [diff] [review]
Patch

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

{ on new line
Attachment #595595 - Flags: review?(luke) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b0466feed19
Target Milestone: --- → mozilla13

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1b0466feed19
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.