Closed Bug 1030699 Opened 7 years ago Closed 7 years ago

IonMonkey: Implement SQRT Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: devillers.nicolas, Assigned: sankha, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.116 Safari/537.36
Blocks: 1003801
Mentor: benj
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8447685 - Flags: review?(benj)
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 ;)
Attachment #8447685 - Flags: review?(benj) → feedback+
Attached patch patchv2 (obsolete) — Splinter Review
Attachment #8447685 - Attachment is obsolete: true
Attachment #8448740 - Flags: review?(benj)
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)
Attachment #8448740 - Flags: review?(benj) → feedback+
Attached patch patchv3 (obsolete) — Splinter Review
Attachment #8448740 - Attachment is obsolete: true
Attachment #8449356 - Flags: review?(benj)
Attached image iongraph output
The iongraph output showing sqrt instruction marked as grey and also specialized for Float32.
Comment on attachment 8449356 [details] [diff] [review]
patchv3

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

Awesome, thanks!
Attachment #8449356 - Flags: review?(benj) → review+
Attached patch patch v3 rebased (obsolete) — Splinter Review
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+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/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.