Closed Bug 1024896 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement Round Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

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

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

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++]
Working on it!
Assignee: nobody → guillaume.turri
Mentor: benj → nicolas.b.pierron
Attached patch bug1024896.patch (obsolete) — Splinter Review
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+
Attached patch bug1024896_2.patch (obsolete) — Splinter Review
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/
Attachment #8444044 - Attachment is obsolete: true
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
Closed: 10 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.