1011539 - IonMonkey: Implement Sub Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Implement RSub in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Comment 1

[Amol Mundayoor]

Hi Nicolas,

I would like to work on this as my first bug. I have successfully built SpiderMonkey and ran jit-tests on the js shell with all pass. What should be my next steps?

Thanks for the opportunity!

Sincerely,
Amol

Comment 2

[Sankha Narayan Guria [:sankha]]

Attachment: patchv1

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8424456 [details] [diff] [review]
patchv1

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

Have you checked the result of iongraph?
Can you also attach the png which highlight that RSub are correctly flagged as recover instruction in the rsub_float function.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +117,5 @@
> +var uceFault_sub_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_sub_float'));
> +function rsub_float(i) {
> +    var t = Math.fround(1/3);
> +    var fi = Math.fround(i);
> +    var x = Math.fround(Math.fround(Math.fround(Math.fround(t - fi) + t) - fi) + t);

Can we make this test only depend on RSub and not RAdd, otherwise as RAdd is correct, we would not be able to check any error in the result of the substraction.

@@ +119,5 @@
> +    var t = Math.fround(1/3);
> +    var fi = Math.fround(i);
> +    var x = Math.fround(Math.fround(Math.fround(Math.fround(t - fi) + t) - fi) + t);
> +    if (uceFault_sub_float(i) || uceFault_sub_float(i))
> +        assertEq(x, -197);

Comment with details operations.

@@ +133,5 @@
>      radd_float(i);
>      radd_string(i);
>      radd_object(i);
> +    rsub_number(i);
> +    rsub_float(i);

Add rsub_object too.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to amol.com from comment #1)
> I would like to work on this as my first bug. I have successfully built
> SpiderMonkey and ran jit-tests on the js shell with all pass. What should be
> my next steps?

Great, if you don't mind I will let you work on RMod (Bug 1012632), which is basically the same idea, except that this is for the modulo operator.

Comment 5

[Inanc Seylan]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> 
> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +117,5 @@
> > +var uceFault_sub_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_sub_float'));
> > +function rsub_float(i) {
> > +    var t = Math.fround(1/3);
> > +    var fi = Math.fround(i);
> > +    var x = Math.fround(Math.fround(Math.fround(Math.fround(t - fi) + t) - fi) + t);
> 
> Can we make this test only depend on RSub and not RAdd, otherwise as RAdd is
> correct, we would not be able to check any error in the result of the
> substraction.
> 
> @@ +119,5 @@
> > +    var t = Math.fround(1/3);
> > +    var fi = Math.fround(i);
> > +    var x = Math.fround(Math.fround(Math.fround(Math.fround(t - fi) + t) - fi) + t);
> > +    if (uceFault_sub_float(i) || uceFault_sub_float(i))
> > +        assertEq(x, -197);
> 
> Comment with details operations.
> 

Would the following be a reasonable test for avoiding the RAdd? Or the numbers look too ugly?

var x = Math.fround(Math.fround(Math.fround(Math.fround(fi - t) - t) - fi) - t);
if (uceFault_sub_float(i) || uceFault_sub_float(i))
    assertEq(x, -1.0000051259994507); /* != -1.0000000298023224 (when computed with double subtractions) */

Comment 6

[Nicolas B. Pierron [:nbp]]

Note that Sankha is currently assigned & working on this bug, he will come back to it tonight.

(In reply to Inanc Seylan from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > 
> > ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> > @@ +117,5 @@
> > > +var uceFault_sub_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_sub_float'));
> > > +function rsub_float(i) {
> > > +    var t = Math.fround(1/3);
> > > +    var fi = Math.fround(i);
> > > +    var x = Math.fround(Math.fround(Math.fround(Math.fround(t - fi) + t) - fi) + t);
> > 
> > Can we make this test only depend on RSub and not RAdd, otherwise as RAdd is
> > correct, we would not be able to check any error in the result of the
> > substraction.
> 
> Would the following be a reasonable test for avoiding the RAdd? Or the
> numbers look too ugly?
> 
> var x = Math.fround(Math.fround(Math.fround(Math.fround(fi - t) - t) - fi) -
> t);
> if (uceFault_sub_float(i) || uceFault_sub_float(i))
>     assertEq(x, -1.0000051259994507); /* != -1.0000000298023224 (when
> computed with double subtractions) */

This is the way to go as these do not depend on RAdd, and only sequences of float operations can highlight the differences between float and double computations.  So by alternating RAdd and RSub, might have masked the missing truncations.

Comment 7

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: iongraph output

This PNG shows the sub instruction in gray like resume points.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8428867 [details] [diff] [review]
patch v2

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

This sounds good.

Comment 10

[Sankha Narayan Guria [:sankha]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=751c4f034b6b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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