Last Comment Bug 900125 - Float32: add Math.fround to the interpreter
: Float32: add Math.fround to the interpreter
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla26
Assigned To: Benjamin Bouvier [:bbouvier]
:
Mentors:
Depends on:
Blocks: 900120 900257 904918
  Show dependency treegraph
 
Reported: 2013-07-31 11:23 PDT by Benjamin Bouvier [:bbouvier]
Modified: 2013-12-05 14:57 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
26+


Attachments
WIP (2.78 KB, patch)
2013-07-31 11:58 PDT, Benjamin Bouvier [:bbouvier]
no flags Details | Diff | Review
Proposed implementation (2.72 KB, patch)
2013-07-31 17:41 PDT, Benjamin Bouvier [:bbouvier]
sstangl: review+
Details | Diff | Review
Tests (1.16 KB, patch)
2013-07-31 17:44 PDT, Benjamin Bouvier [:bbouvier]
jwalden+bmo: review+
Details | Diff | Review
More tests (3.21 KB, patch)
2013-08-02 15:38 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Review
1 - Adds roundFloat32 to the interpreter (2.73 KB, patch)
2013-08-20 15:41 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Review
2 - Tests (3.31 KB, patch)
2013-08-20 15:42 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Review
1 - Adds fround to the interpreter (2.72 KB, patch)
2013-08-28 18:08 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Review
2 - Tests (3.18 KB, patch)
2013-08-28 18:09 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Review

Description Benjamin Bouvier [:bbouvier] 2013-07-31 11:23:53 PDT
Math.ToFloat32 takes a JS value and converts it to a Float32, whenever possible.
This function has been successfully proposed to TC39 recently.
Comment 1 Benjamin Bouvier [:bbouvier] 2013-07-31 11:58:44 PDT
Created attachment 783894 [details] [diff] [review]
WIP

This is actually trivial.

I see that some math functions belong to the js:: namespace and some other don't.
Waldo or luke, is there any criteria of choice to prefer where to put the function?

Luke, should I add a browser pref for this feature, as it hasn't been accepted by the committee yet?

Waldo, I would like to add some tests for this function. I obviously understand that I should put them in /js/src/tests/, but is there any specific subdirectory where to put them?
Comment 2 Luke Wagner [:luke] 2013-07-31 12:25:33 PDT
It sounds like there was general support at the most recent TC39 meeting; I forget Dave, was it added to todo list of ES6?  Given that, it seems like, as with Math.imul, we have good reason to add it w/o a pref.  Agreed Dave?  Also is there any strawman/wiki URL we can link to here?
Comment 3 Luke Wagner [:luke] 2013-07-31 12:26:46 PDT
Regarding the js:: question: we are incrementally moving all the things to namespace js and un-prefixing, so let's do that (you can put it righ tnext to js::math_imul).  (If you'd like to write a patch to move the remaining extern js_* functions to js::, go for it :)
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-31 13:24:27 PDT
The JS engine can't depend on browser prefs anyway, because it's browser that depends on js/src, not the other way around.  (We could compile-time pref it based on update channel, I think, but that's slightly messy, and best avoided if we can, as it seems like is probably possible here.)
Comment 5 Benjamin Bouvier [:bbouvier] 2013-07-31 17:41:55 PDT
Created attachment 784118 [details] [diff] [review]
Proposed implementation

Same implementation, just the order of functions has been changed.
Comment 6 Benjamin Bouvier [:bbouvier] 2013-07-31 17:44:01 PDT
Created attachment 784119 [details] [diff] [review]
Tests

A few tests. As there is no formal spec right now, I just considered some obvious cases, so I would love to get feedback and / or ideas regarding some other nice tests to add.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-01 18:04:39 PDT
Comment on attachment 784119 [details] [diff] [review]
Tests

Review of attachment 784119 [details] [diff] [review]:
-----------------------------------------------------------------

Et viola!  ;-)

Probably you could go even further than I suggested, I'm just covering the obvious edge-case bases that occurred to me with a few minutes' thinking.  (And a few hours' drawing up, grrrr.)

::: js/src/tests/ecma_6/Math/toFloat32.js
@@ +1,1 @@
> +// Some tests regarding conversion to Float32

Put a public-domain license block at the top of this -- see js/src/tests/ecma_6/Map/NaN-as-key.js for an example.

@@ +4,5 @@
> +assertEq(Math.toFloat32(NaN), NaN);
> +assertEq(Math.toFloat32(-Infinity), -Infinity);
> +assertEq(Math.toFloat32(Infinity), Infinity);
> +assertEq(Math.toFloat32(-0), -0);
> +assertEq(Math.toFloat32(+0), +0);

We should test the maximum number (float64/double) value and the maximum float32 value:

  function maxValue(exponentWidth, significandWidth)
  {
    var n = 0;
    var maxExp = Math.pow(2, exponentWidth - 1) - 1;
    for (var i = significandWidth; i >= 0; i--)
      n += Math.pow(2, maxExp - i);
    return n;
  }

  var DBL_MAX = maxValue(11, 52);
  assertEq(DBL_MAX, Number.MAX_VALUE); // sanity check

  // Finite as a double, too big for a float
  assertEq(Math.toFloat32(DBL_MAX), Infinity);

  var FLT_MAX = maxValue(8, 23);
  assertEq(Math.toFloat32(FLT_MAX), FLT_MAX);

  assertEq(Math.toFloat32(FLT_MAX + Math.pow(2, Math.pow(2, 8 - 1) - 1 - 23 - 2)), FLT_MAX); // round-nearest rounds down to FLT_MAX
  assertEq(Math.toFloat32(FLT_MAX + Math.pow(2, Math.pow(2, 8 - 1) - 1 - 23 - 1)), Infinity); // no longer rounds down to FLT_MAX

We should also test some denormals:

  function minValue(exponentWidth, significandWidth)
  {
    return Math.pow(2, -(Math.pow(2, exponentWidth - 1) - 2) - significandWidth);
  }

  var DBL_MIN = Math.pow(2, -1074);
  assertEq(DBL_MIN, Number.MIN_VALUE); // sanity check

  // Too small for a float
  assertEq(Math.toFloat32(DBL_MIN), 0);

  var FLT_MIN = minValue(8, 23);
  assertEq(Math.toFloat32(FLT_MIN), FLT_MIN);

  assertEq(Math.toFloat32(FLT_MIN / 2), 0); // halfway, round-nearest rounds down to 0 (even)
  assertEq(Math.toFloat32(FLT_MIN / 2 + Math.pow(2, -202)), FLT_MIN); // first double > FLT_MIN / 2, rounds up to FLT_MIN

Also some tests that round (even-directed and non-even-directed) to negative zero would be good:

  assertEq(Math.toFloat32(-FLT_MIN), -FLT_MIN);

  assertEq(Math.toFloat32(-FLT_MIN / 2), -0); // halfway, round-nearest rounds up to -0 (even)
  assertEq(Math.toFloat32(-FLT_MIN / 2 - Math.pow(2, -202)), -FLT_MIN); // first double < -FLT_MIN / 2, rounds down to -FLT_MIN

I think all that code will just work, as I did a bunch of manual testing of it (not using your patch, just toFloat32-like operations), but you should test it and be sure.
Comment 8 Benjamin Bouvier [:bbouvier] 2013-08-02 15:38:02 PDT
Created attachment 785240 [details] [diff] [review]
More tests

Awesome tests, Waldo!
They all pass with Math.toFloat32 in the interpreter and also with --ion-eager and the patch in bug 900257.

Yay \o/

Carrying over r+.
Comment 9 Sean Stangl [:sstangl] 2013-08-15 11:18:28 PDT
Comment on attachment 784118 [details] [diff] [review]
Proposed implementation

Review of attachment 784118 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsmath.cpp
@@ +429,5 @@
> +        return false;
> +
> +    float f = x;
> +    x = f;
> +    args.rval().setNumber(x);

setDouble((double)f)
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-15 11:23:15 PDT
double(f) or static_cast<double>(f) -- I'd go with the former -- would be better than a C-style cast.
Comment 11 Dave Herman [:dherman] 2013-08-19 12:29:31 PDT
This is my first draft of a spec for Math.toFloat32:

    https://etherpad.mozilla.org/Math-toFloat32

I've got emails out to various stakeholders to check my thinking. Note that there's an implicit ToNumber coercion applied to the argument on entry to the function.

Dave
Comment 12 Luke Wagner [:luke] 2013-08-20 11:50:36 PDT
Note: naming change in the spec to Math.roundFloat32 since it was pointed out that all other toX() funtions are methods on a prototype acting on 'this', so toFloat32 didn't fit in.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-20 12:07:58 PDT
(In reply to Dave Herman [:dherman] from comment #11)

After the initial ToNumber on the argument, why wouldn't you delegate to IEEE-754?  Something like

Math.toFloat32(x):
1. Let x be ToNumber(x).
2. Let y be x, converted to single format according to the IEEE-754 specification, using the round to nearest mode.  NOTE: The number y may not be the same as the number x, and it may be infinite when x is finite.  If x is a NaN value, the bit pattern of y is unspecified.
3. Return y.

or so.  Trying to write out an algorithm for this, rather than delegating to IEEE-754, is just asking to get something wrong.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-20 12:09:31 PDT
Not that it fits well with asm.js (but I guess that's our problem to solve, not the spec's), but wouldn't it be more idiomatically JS for the method to be Number.prototype.toFloat32(), if the toX concern is valid (which it is)?
Comment 15 Luke Wagner [:luke] 2013-08-20 12:36:30 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
Allen made that point.  Yeah, Number.prototype.toFloat32 is just inherently harder for asm.js to lock down so that, after an initial check, you don't need to check anymore (you can, but it's gnarly and it would be slow on normal engines).
Comment 16 Benjamin Bouvier [:bbouvier] 2013-08-20 15:41:16 PDT
Created attachment 793162 [details] [diff] [review]
1 - Adds roundFloat32 to the interpreter

carrying over r+ from sstangl (fixed conversion).

This currently implements what Waldo said (modulo the fact that if there's no argument, NaN is returned).
Comment 17 Benjamin Bouvier [:bbouvier] 2013-08-20 15:42:42 PDT
Created attachment 793163 [details] [diff] [review]
2 - Tests

Carrying over r+ from Waldo.

Added a test that Math.roundFloat32() == NaN.
Comment 18 Luke Wagner [:luke] 2013-08-23 15:24:03 PDT
More permanent link: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.8.2.34
Comment 19 Benjamin Bouvier [:bbouvier] 2013-08-28 18:08:29 PDT
Created attachment 796998 [details] [diff] [review]
1 - Adds fround to the interpreter

After discussions, it looks like the name of this function should be Math.fround, for Float rounding. This is consistent with libc's math library. The name can still be changed in the future in a follow-up bug.

Carrying over r+ from sstangl
Comment 20 Benjamin Bouvier [:bbouvier] 2013-08-28 18:09:20 PDT
Created attachment 796999 [details] [diff] [review]
2 - Tests

Carrying over r+ from Waldo.
Comment 24 Jean-Yves Perrier [:teoli] 2013-09-03 21:45:49 PDT
Thank you very much, Benjamin. A small info: once documentation is done, we don't remove the dev-doc-needed flag, we switch it to dev-doc-complete, that we can continue to keep track of it ;-)
Comment 25 Florian Bender 2013-11-24 12:46:31 PST
Nom'ing for relnote 26.

Note You need to log in before you can comment on or make changes to this bug.