IonMonkey: Implement Round Recover Instruction

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbouvier, Assigned: guillaume.turri, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla33
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Mentor: benj
Whiteboard: [good first bug][mentor=bbouvier][lang=c++] → [good first bug][lang=c++]
(Assignee)

Comment 1

5 years ago
Working on it!
Assignee: nobody → guillaume.turri
Mentor: benj → nicolas.b.pierron
(Assignee)

Comment 2

5 years ago
Created attachment 8444035 [details] [diff] [review]
bug1024896.patch
Attachment #8444035 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8444035 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 8444044 [details] [diff] [review]
bug1024896_2.patch
Attachment #8444035 - Attachment is obsolete: true
Attachment #8444044 - Flags: review?(nicolas.b.pierron)
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*).
Attachment #8444044 - Flags: review?(nicolas.b.pierron) → feedback+
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/
(Assignee)

Comment 7

5 years ago
Created attachment 8444054 [details] [diff] [review]
bug1024896_3.patch
Attachment #8444044 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8444054 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8444054 - Flags: review?(nicolas.b.pierron) → review+
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
https://hg.mozilla.org/mozilla-central/rev/efaa172c0ee4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.