1024896 - IonMonkey: Implement Round Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Implement RRound in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

MRound is the operation triggered by Math.round(double) -> int32.

Comment 1

[guillaume.turri]

Working on it!

Comment 2

[guillaume.turri]

Attachment: bug1024896.patch

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444035 [details] [diff] [review]
bug1024896.patch

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

This looks good :)
Still a few things to fix.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +164,5 @@
>  }
>  
> +var uceFault_round = eval(uneval(uceFault).replace('uceFault', 'uceFault_round'));
> +function round_int(i) {
> +    var x = Math.round(i+1.4);

nit: add spaces around the "+".

::: js/src/jit/Recover.cpp
@@ +410,5 @@
> +  
> +    MOZ_ASSERT(!arg.isObject());
> +    if(!js::math_round_d(cx, arg, &result))
> +      return false;
> +  

style-nit: remove trailing white spaces in this function. (and also above)

::: js/src/jit/Recover.h
@@ +32,5 @@
>      _(Mod)                                      \
>      _(Concat)                                   \
>      _(NewObject)                                \
> +    _(NewDerivedTypedObject)                    \
> +    _(Round)

For all files, follow the same order as in the list of instructions in Bug 1003801.
At least to make sure that they are in the same order in all files.

::: js/src/jsmath.cpp
@@ +781,5 @@
> +    double d;
> +    if (!math_round_d(cx, arg, &d))
> +        return false;
> +
> +    res.setDouble(d);

Use setNumber.

@@ +786,5 @@
> +    return true;
> +}
> +
> +bool
> +js::math_round_d(JSContext *cx, HandleValue v, double *out){

see the comment below and move the content of this function in the previous one.

@@ +838,3 @@
>          return false;
>  
> +    args.rval().setNumber(x);

Remove this line, and replace the "&x" by "args.rval()".

This will use the MutableHandle variant of the function, instead of the double* one which are doing the same thing.

Comment 4

[guillaume.turri]

Attachment: bug1024896_2.patch

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444044 [details] [diff] [review]
bug1024896_2.patch

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

Ok, a last pass of renaming and this would be good :)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +163,5 @@
>      return i;
>  }
>  
> +var uceFault_round = eval(uneval(uceFault).replace('uceFault', 'uceFault_round'));
> +function round_int(i) {

nit: s/round_int/round_number/

::: js/src/jit/Recover.h
@@ +275,5 @@
> +    RINSTRUCTION_HEADER_(Round)
> +
> +   virtual uint32_t numOperands() const {
> +     return 1;
> +   }

style-nit: properly indent this function.

::: js/src/jsmath.cpp
@@ +776,5 @@
>      return true;
>  }
>  
> +bool
> +js::math_roundd(JSContext *cx, HandleValue arg, MutableHandleValue res){

style-nit: Move the curly brace to the next line.

@@ +779,5 @@
> +bool
> +js::math_roundd(JSContext *cx, HandleValue arg, MutableHandleValue res){
> +    double d;
> +
> +    if (!ToNumber(cx, arg, &d))

style-nit: as "d" is initialize by the condition, I'll suggest to remove the additional line after "d"'s declaration.

::: js/src/jsmath.h
@@ +274,5 @@
>  extern double
>  math_floor_impl(double x);
>  
>  extern bool
> +math_roundd(JSContext *cx, HandleValue arg, MutableHandleValue res);

nit: I think it would make more sense to name it  math_round_handle, as we would have the same problem as for other math primitive and that both variant  "math_round" and "math_round_impl" would have ambiguous use cases as we are casting these to (void*).

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444044 [details] [diff] [review]
bug1024896_2.patch

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +163,5 @@
>      return i;
>  }
>  
> +var uceFault_round = eval(uneval(uceFault).replace('uceFault', 'uceFault_round'));
> +function round_int(i) {

nit: s/round_number/rround_number/

Comment 7

[guillaume.turri]

Attachment: bug1024896_3.patch

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444054 [details] [diff] [review]
bug1024896_3.patch

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

This sounds good, I will land this change on Try soon.

Comment 9

[Nicolas B. Pierron [:nbp]]

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

Comment 10

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efaa172c0ee4

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 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efaa172c0ee4