1030699 - IonMonkey: Implement SQRT Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[devillers.nicolas]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.116 Safari/537.36

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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/jit-test/tests/ion/dce-with-rinstructions.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 ;)

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: patchv2

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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/jit-test/tests/ion/dce-with-rinstructions.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)

Comment 5

[Sankha Narayan Guria [:sankha]]

Attachment: patchv3

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: iongraph output

The iongraph output showing sqrt instruction marked as grey and also specialized for Float32.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8449356 [details] [diff] [review]
patchv3

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

Awesome, thanks!

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: patch v3 rebased

try: https://tbpl.mozilla.org/?tree=Try&rev=93b93bc7b348

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 10

[Sankha Narayan Guria [:sankha]]

Attachment: patch v3 rebased

try: https://tbpl.mozilla.org/?tree=Try&rev=b998178aea86

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5ef969405c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab5ef969405c