1028573 - IonMonkey: Implement MathFunction[Round] Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[David Moreira [:vash]]

Implement the MathFunction (Random for double).
See Bug 1003801 comment 0 for explanation.

Comment 1

[David Moreira [:vash]]

Attachment: mathfunction-round.patch

To apply on top of the patch submitted in Bug 1024896 Comment 7.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444060 [details] [diff] [review]
mathfunction-round.patch

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

This looks Great :)
Still some fixes to do.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +172,5 @@
>  }
>  
> +var uceFault_round_double = eval(uneval(uceFault).replace('uceFault', 'uceFault_round_double'));
> +function rround_double(i) {
> +    var x = Math.round(i - (-1 >>> 0));

nit: use "i +" instead of "i -", this is just clear that we overflow the Int32 range this way.

@@ +174,5 @@
> +var uceFault_round_double = eval(uneval(uceFault).replace('uceFault', 'uceFault_round_double'));
> +function rround_double(i) {
> +    var x = Math.round(i - (-1 >>> 0));
> +    if (uceFault_round_double(i) || uceFault_round_double(i))
> +        assertEq(x, -4294967196); /* = -(2 ^ 32 + 100) */

nit: Instead of "-4294967196", use "99 + (-1 >>> 0)".

::: js/src/jit/Recover.cpp
@@ +416,5 @@
>      return true;
>  }
>  
>  bool
> +MMathFunction::writeRecoverData(CompactBufferWriter &writer) const

nit: Move this function under concat, as it would be extended later with its own recover function.

@@ +421,5 @@
> +{
> +    MOZ_ASSERT(canRecoverOnBailout());
> +    switch (function_) {
> +    case Round:
> +        writer.writeUnsigned(uint32_t(RInstruction::Recover_Round));

style-nit: case should be indented by 2, withing the scope of the switch.

Comment 3

[David Moreira [:vash]]

Attachment: mathfunction-round-2.patch

To apply on top of the patch submitted in Bug 1024896 Comment 7.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444072 [details] [diff] [review]
mathfunction-round-2.patch

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +174,5 @@
> +var uceFault_round_double = eval(uneval(uceFault).replace('uceFault', 'uceFault_round_double'));
> +function rround_double(i) {
> +    var x = Math.round(i + (-1 >>> 0));
> +    if (uceFault_round_double(i) || uceFault_round_double(i))
> +        assertEq(x, 99 + (-1 >>> 0)); /* = i + 2 ^ 32 */

nit: /* = i + 2 ^ 32 - 1 (double) */

Comment 5

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=b9a4bebbe04d

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80b2b07d4dd6

I've landed your patch as part of the contribution of others.  You will find the result of it on our continuous integration system (tbpl) [1].  Later a Sheriff will take the patches and merge into mozilla-central.


[1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ec5b6129445b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80b2b07d4dd6

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

For the records, I've wrapped my head around potential float32 issues. If the MRound is specialized into a Float32 operation, do we need to convert the result into a float32 explicitly or not? After some discussion with nbp, Waldo and mjrosenb, this can't happen for the following reasons:

- The result of roundf cannot escape the Float32 range, as it's rounding towards zero (in particular, it cannot reach infinities, as ceilf and floorf can do).
- All Float32 can be precisely represented as Float64 (including Float32 subnormals, which need not be Float64 subnormals). (*)
- For all float x (non NaN), Round(ToDouble(x)) == ToDouble(Roundf(x)), which is slightly more powerful than the initial Float32 "commutativity" property. This has been tested on the entire Float32 space on a C program.

In RRound::recover, the input is already a Float64, which is the exact representation of the Float32 input, as per (*). By the last fact, there cannot be any error with Float32 and the MMathFunction[Round] recover instruction. Please don't hesitate to correct me if I am wrong; I have tried making failing test cases for an hour before understanding they shouldn't happen.