Closed Bug 1028704 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement MinMax Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: tandiap, Assigned: tandiap, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Implement RMinMax in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Blocks: 1003801
Assignee: nobody → patandia
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch 1028704.patch (obsolete) — Splinter Review
Attachment #8448441 - Flags: review?(nicolas.b.pierron)
Attachment #8448441 - Flags: review?(nicolas.b.pierron)
Attached patch 1028704.patch (obsolete) — Splinter Review
Attachment #8448441 - Attachment is obsolete: true
Attachment #8448446 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8448446 [details] [diff] [review]
1028704.patch

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

This sounds good :)
Still some name changes and a few nits, and it would be good for landing ;)

::: js/src/jit/Recover.h
@@ +351,5 @@
> +{
> +  private:
> +    bool isMax_;
> +  public:
> +    RINSTRUCTION_HEADER_(MinMax)

nit: Add a new line before "public:".

::: js/src/jsmath.cpp
@@ +552,5 @@
>      return true;
>  }
>  
> +inline double
> +max_double(double x, double y) {

nit: s/inline/static/, inline is just a hint for the compiler, and the symbol might be exported.  Static is another hint for the compiler that the symbol only exist in this translation unit, and thus the call can be optimized away.

style-nit: move the open braces on a new line.

@@ +603,5 @@
>      return true;
>  }
>  
> +bool
> +minmax_handle(bool max, JSContext *cx, HandleValue a, HandleValue b, MutableHandleValue res)

prefix it with js_
replace the _handle by _impl, as there is no ambiguity around the name.
Attachment #8448446 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch 1028704.patchSplinter Review
Attachment #8448446 - Attachment is obsolete: true
Attachment #8448986 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8448986 [details] [diff] [review]
1028704.patch

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

Except for the following style nits, this sounds good :)
I will fix these 2 style-nits and send this patch to try & land it once everything is green.

Is there any other bug that you might want to work on?

::: js/src/jsmath.cpp
@@ +557,5 @@
> +{
> +    // Math.max(num, NaN) => NaN, Math.max(-0, +0) => +0
> +    if (x > y || IsNaN(x) || (x == y && IsNegative(y))) {
> +        return x;
> +    }

style-nit: do not use braces for one line then-blocks.

@@ +583,5 @@
> +{
> +    // Math.min(num, NaN) => NaN, Math.min(-0, +0) => -0
> +    if (x < y || IsNaN(x) || (x == y && IsNegativeZero(x))) {
> +        return x;
> +    }

style-nit: same here.
Attachment #8448986 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8448986 [details] [diff] [review]
1028704.patch

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

::: js/src/jsmath.cpp
@@ +604,5 @@
>      return true;
>  }
>  
> +bool
> +js_minmax_impl(bool max, JSContext *cx, HandleValue a, HandleValue b, MutableHandleValue res)

Note: we usually put the JSContext *cx as the first argument.  I will fix that before pushing.
Comment on attachment 8448986 [details] [diff] [review]
1028704.patch

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

::: js/src/jit/Recover.cpp
@@ +730,5 @@
> +    RootedValue a(cx, iter.read());
> +    RootedValue b(cx, iter.read());
> +    RootedValue result(cx);
> +
> +    if(!js_minmax_impl(isMax_, cx, a, b, &result))

style-nit: we add space after the "if␣("

::: js/src/jsmath.cpp
@@ +613,5 @@
> +        return false;
> +    if(!ToNumber(cx, b, &y))
> +        return false;
> +
> +    if(max)

style-nit: same here and above.
https://hg.mozilla.org/mozilla-central/rev/924708eb8375
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.