Closed
Bug 1028704
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement MinMax Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: tandiap, Assigned: tandiap, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
8.83 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Implement RMinMax in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanation.
Updated•10 years ago
|
Assignee: nobody → patandia
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8448441 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•10 years ago
|
Attachment #8448441 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8448441 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8448446 -
Flags: review?(nicolas.b.pierron)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8448446 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8448986 -
Flags: review?(nicolas.b.pierron)
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fb0a4c7f39b4
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•