Closed Bug 1011539 Opened 6 years ago Closed 6 years ago

IonMonkey: Implement Sub Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nbp, Assigned: sankha)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=nbp][lang=c++])

Attachments

(2 files, 1 obsolete file)

Implement RSub in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
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
Attached patch patchv1 (obsolete) — Splinter Review
Attachment #8424456 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8424456 - Flags: review?(nicolas.b.pierron)
(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.
Assignee: nobody → sankha93
(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) */
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.
Attached patch patch v2Splinter Review
Attachment #8424456 - Attachment is obsolete: true
Attachment #8428867 - Flags: review?(nicolas.b.pierron)
Attached image iongraph output
This PNG shows the sub instruction in gray like resume points.
Comment on attachment 8428867 [details] [diff] [review]
patch v2

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

This sounds good.
Attachment #8428867 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/ca90df6c5130
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.