Closed
Bug 900125
Opened 7 years ago
Closed 7 years ago
Float32: add Math.fround to the interpreter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Not set
Tracking
()
RESOLVED
FIXED
mozilla26
Tracking  Status  

relnotefirefox    26+ 
People
(Reporter: bbouvier, Assigned: bbouvier)
References
(Blocks 1 open bug)
Details
(Keywords: devdoccomplete)
Attachments
(2 files, 6 obsolete files)
2.72 KB,
patch

bbouvier
:
review+

Details  Diff  Splinter Review 
3.18 KB,
patch

bbouvier
:
review+

Details  Diff  Splinter Review 
Math.ToFloat32 takes a JS value and converts it to a Float32, whenever possible. This function has been successfully proposed to TC39 recently.
Assignee  
Comment 1•7 years ago


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?
Flags: needinfo?(luke)
Flags: needinfo?(jwalden+bmo)
Comment 2•7 years ago


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?
Flags: needinfo?(luke)
Comment 3•7 years ago


Regarding the js:: question: we are incrementally moving all the things to namespace js and unprefixing, 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•7 years ago


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 compiletime 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.)
Flags: needinfo?(jwalden+bmo)
Assignee  
Comment 5•7 years ago


Same implementation, just the order of functions has been changed.
Attachment #783894 
Attachment is obsolete: true
Attachment #784118 
Flags: review?(sstangl)
Assignee  
Comment 6•7 years ago


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.
Attachment #784119 
Flags: review?(jwalden+bmo)
Updated•7 years ago

Flags: needinfo?(dherman)
Comment 7•7 years ago


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 edgecase 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 publicdomain license block at the top of this  see js/src/tests/ecma_6/Map/NaNaskey.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); // roundnearest 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, roundnearest 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 (evendirected and nonevendirected) to negative zero would be good: assertEq(Math.toFloat32(FLT_MIN), FLT_MIN); assertEq(Math.toFloat32(FLT_MIN / 2), 0); // halfway, roundnearest 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 toFloat32like operations), but you should test it and be sure.
Attachment #784119 
Flags: review?(jwalden+bmo) → review+
Assignee  
Comment 8•7 years ago


Awesome tests, Waldo! They all pass with Math.toFloat32 in the interpreter and also with ioneager and the patch in bug 900257. Yay \o/ Carrying over r+.
Attachment #784119 
Attachment is obsolete: true
Attachment #785240 
Flags: review+
Comment 9•7 years ago


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)
Attachment #784118 
Flags: review?(sstangl) → review+
Comment 10•7 years ago


double(f) or static_cast<double>(f)  I'd go with the former  would be better than a Cstyle cast.
Comment 11•7 years ago


This is my first draft of a spec for Math.toFloat32: https://etherpad.mozilla.org/MathtoFloat32 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
Flags: needinfo?(dherman)
Comment 12•7 years ago


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.
Updated•7 years ago

Summary: Float32: add Math.ToFloat32 to the interpreter → Float32: add Math.roundFloat32 to the interpreter
Comment 13•7 years ago


(In reply to Dave Herman [:dherman] from comment #11) After the initial ToNumber on the argument, why wouldn't you delegate to IEEE754? Something like Math.toFloat32(x): 1. Let x be ToNumber(x). 2. Let y be x, converted to single format according to the IEEE754 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 IEEE754, is just asking to get something wrong.
Comment 14•7 years ago


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•7 years ago


(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).
Assignee  
Comment 16•7 years ago


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).
Attachment #784118 
Attachment is obsolete: true
Attachment #793162 
Flags: review+
Assignee  
Comment 17•7 years ago


Carrying over r+ from Waldo. Added a test that Math.roundFloat32() == NaN.
Attachment #785240 
Attachment is obsolete: true
Attachment #793163 
Flags: review+
Comment 18•7 years ago


More permanent link: http://people.mozilla.org/~jorendorff/es6draft.html#sec15.8.2.34
Assignee  
Comment 19•7 years ago


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 followup bug. Carrying over r+ from sstangl
Attachment #793162 
Attachment is obsolete: true
Attachment #796998 
Flags: review+
Assignee  
Comment 20•7 years ago


Carrying over r+ from Waldo.
Attachment #793163 
Attachment is obsolete: true
Attachment #796999 
Flags: review+
Assignee  
Comment 21•7 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/fefe46f36a5b https://hg.mozilla.org/integration/mozillainbound/rev/1882f24d48d5
Comment 22•7 years ago


https://hg.mozilla.org/mozillacentral/rev/fefe46f36a5b https://hg.mozilla.org/mozillacentral/rev/1882f24d48d5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution:  → FIXED
Target Milestone:  → mozilla26
Updated•7 years ago

Summary: Float32: add Math.roundFloat32 to the interpreter → Float32: add Math.fround to the interpreter
Updated•7 years ago

Keywords: devdocneeded
Assignee  
Comment 23•7 years ago


https://developer.mozilla.org/enUS/docs/Web/JavaScript/Reference/Global_Objects/Math https://developer.mozilla.org/enUS/docs/JavaScript/Reference/Global_Objects/Math/fround https://developer.mozilla.org/enUS/docs/Mozilla/Firefox/Releases/26
Keywords: devdocneeded
Comment 24•7 years ago


Thank you very much, Benjamin. A small info: once documentation is done, we don't remove the devdocneeded flag, we switch it to devdoccomplete, that we can continue to keep track of it ;)
Keywords: devdoccomplete
Updated•6 years ago

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