Closed Bug 1028573 Opened 7 years ago Closed 7 years ago

IonMonkey: Implement MathFunction[Round] Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: vash, Assigned: vash, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Implement the MathFunction (Random for double).
See Bug 1003801 comment 0 for explanation.
Assignee: nobody → davidmoreirafr
Blocks: 1003801
OS: Linux → All
Hardware: x86_64 → All
Summary: IonMonkey: Implement MathFunction[Random] Recover Instruction → IonMonkey: Implement MathFunction[Round] Recover Instruction
Attached patch mathfunction-round.patch (obsolete) — Splinter Review
To apply on top of the patch submitted in Bug 1024896 Comment 7.
Attachment #8444060 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8444060 - Flags: review?(nicolas.b.pierron) → feedback+
To apply on top of the patch submitted in Bug 1024896 Comment 7.
Attachment #8444060 - Attachment is obsolete: true
Attachment #8444072 - Flags: review?(nicolas.b.pierron)
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) */
Attachment #8444072 - Flags: review?(nicolas.b.pierron) → review+
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
https://hg.mozilla.org/mozilla-central/rev/80b2b07d4dd6
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
You need to log in before you can comment on or make changes to this bug.