Closed
Bug 1030699
Opened 7 years ago
Closed 7 years ago
IonMonkey: Implement SQRT Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: devillers.nicolas, Assigned: sankha, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
265.53 KB,
image/png

Details  
7.92 KB,
patch

Details  Diff  Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.116 Safari/537.36
Updated•7 years ago

Assignee  
Comment 1•7 years ago


Attachment #8447685 
Flags: review?(benj)
Comment 2•7 years ago


Comment on attachment 8447685 [details] [diff] [review] patch v1 Review of attachment 8447685 [details] [diff] [review]:  Nice start! As Sqrt can be specialized for float32, you'll need to add some more before it's entirely correct, see comments inline :) ::: js/src/jittest/tests/ion/dcewithrinstructions.js @@ +401,5 @@ > + t = 1.5; > + if (uceFault_sqrt_object(i)  uceFault_sqrt_object(i)) > + assertEq(x, Math.sqrt(99)); > + return i; > +} You'll need a test for Float32 as well, see also radd_float and similar tests. ::: js/src/jit/Recover.cpp @@ +653,5 @@ > + RootedValue result(cx); > + > + MOZ_ASSERT(num.isNumber()); > + if (!js_math_sqrt_handle(cx, num, &result)) > + return false; Sqrt can be specialized for float32s, in which case it will return a value that needs a conversion to float32 (as we've lost the information that we have a float32 operation). You'll need to encode the fact that this operation is specialized for float32 when writing recover data, and retrieving this information when recovering. See also MAdd::writeRecoverData, RAdd ctor and RAdd::recover. You'll also need a test for that, of course ;)
Attachment #8447685 
Flags: review?(benj) → feedback+
Assignee  
Comment 3•7 years ago


Attachment #8447685 
Attachment is obsolete: true
Attachment #8448740 
Flags: review?(benj)
Comment 4•7 years ago


Comment on attachment 8448740 [details] [diff] [review] patchv2 Review of attachment 8448740 [details] [diff] [review]:  Thanks! We're getting closer, but the patch still needs a few modifications. You can expect a r+ next time ;) ::: js/src/jittest/tests/ion/dcewithrinstructions.js @@ +394,5 @@ > +} > + > +var uceFault_sqrt_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_sqrt_float')); > +function rsqrt_float(i) { > + var x = Math.fround(Math.sqrt(i)); Unfortunately, this is not enough: the argument i needs to be converted into a float32 using Math.fround too, otherwise the Math.sqrt operation won't be specialized as a Float32 operation. Once you've done that modification, can you check in iongraph that the sqrt is actually specialized as a Float32 operation? @@ +396,5 @@ > +var uceFault_sqrt_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_sqrt_float')); > +function rsqrt_float(i) { > + var x = Math.fround(Math.sqrt(i)); > + if (uceFault_sqrt_float(i)  uceFault_sqrt_float(i)) > + assertEq(x, Math.fround(Math.sqrt(99))); Can you add a comment with the expected value of Math.fround(Math.sqrt(Math.fround(99))) and put the value of Math.sqrt(99) aside, to show how they differ? ::: js/src/jit/Recover.cpp @@ +658,5 @@ > + MOZ_ASSERT(num.isNumber()); > + if (!js_math_sqrt_handle(cx, num, &result)) > + return false; > + > + if (isFloatOperation_ && !RoundFloat32(cx, result, &result)) Can you add a comment above this if condition, as in RAdd::recover, or refer to the comment in RAdd::recover? ::: js/src/jsmath.cpp @@ +877,5 @@ > return true; > } > > bool > +js_math_sqrt_handle(JSContext *cx, HandleValue number, MutableHandleValue result) nit: the js_ prefix is not used anymore, instead we use the js:: namespace. Can you change that here and in jsmath.h as well please? ::: js/src/jsmath.h @@ +107,5 @@ > extern bool > js_math_min(JSContext *cx, unsigned argc, js::Value *vp); > > extern bool > +js_math_sqrt_handle(JSContext *cx, js::HandleValue number, js::MutableHandleValue result); nit: use js:: namespace (see above)
Attachment #8448740 
Flags: review?(benj) → feedback+
Assignee  
Comment 5•7 years ago


Attachment #8448740 
Attachment is obsolete: true
Attachment #8449356 
Flags: review?(benj)
Assignee  
Comment 6•7 years ago


The iongraph output showing sqrt instruction marked as grey and also specialized for Float32.
Comment 7•7 years ago


Comment on attachment 8449356 [details] [diff] [review] patchv3 Review of attachment 8449356 [details] [diff] [review]:  Awesome, thanks!
Attachment #8449356 
Flags: review?(benj) → review+
Assignee  
Comment 8•7 years ago


try: https://tbpl.mozilla.org/?tree=Try&rev=93b93bc7b348
Assignee: nobody → sankha93
Attachment #8449356 
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8450925 
Flags: review+
Comment 9•7 years ago


For what it's worth, these oranges are in a test which doesn't contain any sqrt, so I'd be surprised they're related to your patch.
Assignee  
Comment 10•7 years ago


try: https://tbpl.mozilla.org/?tree=Try&rev=b998178aea86
Attachment #8450925 
Attachment is obsolete: true
Assignee  
Updated•7 years ago

Keywords: checkinneeded
Comment 11•7 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/ab5ef969405c
Keywords: checkinneeded
Comment 12•7 years ago


https://hg.mozilla.org/mozillacentral/rev/ab5ef969405c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution:  → FIXED
Target Milestone:  → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•