Closed
Bug 1030699
Opened 10 years ago
Closed 10 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•10 years ago

Assignee  
Comment 1•10 years ago


Attachment #8447685 
Flags: review?(benj)
Comment 2•10 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•10 years ago


Attachment #8447685 
Attachment is obsolete: true
Attachment #8448740 
Flags: review?(benj)
Comment 4•10 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•10 years ago


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


The iongraph output showing sqrt instruction marked as grey and also specialized for Float32.
Comment 7•10 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•10 years ago


Assignee: nobody → sankha93
Attachment #8449356 
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8450925 
Flags: review+
Comment 9•10 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•10 years ago


Attachment #8450925 
Attachment is obsolete: true
Assignee  
Updated•10 years ago

Keywords: checkinneeded
Comment 11•10 years ago


Keywords: checkinneeded
Comment 12•10 years ago


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
•