1062888 - IonMonkey: Implement RToDouble Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[jody]

Hi,

I wanted to work on this bug. I've built a debug version of the js shell which I then tried to run the IONFLAGS=logs command on it, but I still didn't see the tmp/ion/.json file. I'm not sure how to precede beyond this point without the iongraph output. Tips?

Cheers,

Jody

Comment 2

[jody]

(In reply to jody from comment #1)
> Hi,
> 
> I wanted to work on this bug. I've built a debug version of the js shell
> which I then tried to run the IONFLAGS=logs command on it, but I still
> didn't see the tmp/ion/.json file. I'm not sure how to precede beyond this
> point without the iongraph output. Tips?
> 
> Cheers,
> 
> Jody

I forgot to add that I ran the above with --ion-parallel-compile=off and I got the error message that this is deprecated. So I had to run --ion-offthread-compile=off. 

The complete command was IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js --ion-offthread-compile=off --ion-eager ./jit-test/tests/ion/dce-with-rinstructions.js

It ran without errors but did not produce a report.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to jody from comment #2)
> (In reply to jody from comment #1)
> > Hi,
> > 
> > I wanted to work on this bug. I've built a debug version of the js shell
> > which I then tried to run the IONFLAGS=logs command on it, but I still
> > didn't see the tmp/ion/.json file. I'm not sure how to precede beyond this
> > point without the iongraph output. Tips?
> > 
> > Cheers,
> > 
> > Jody
> 
> I forgot to add that I ran the above with --ion-parallel-compile=off and I
> got the error message that this is deprecated. So I had to run
> --ion-offthread-compile=off. 
> 
> The complete command was IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js
> --ion-offthread-compile=off --ion-eager
> ./jit-test/tests/ion/dce-with-rinstructions.js

This should be correct, this should produce a file in /tmp/ named ion.json.
This file would then be used by iongraph without having to do anything.

Can you use gdb to verify that the following line is well executed:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitSpewer.cpp#128

Comment 4

[Peter]

Attachment: Add support for recovering a double value.

This patch enables recovering from IonMonkey to baseline for double values by:

  - adding the RToDouble class
  - updating the MToDouble class
  - adding two tests in dce-with-rinstructions.js

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8515539 [details] [diff] [review]
Add support for recovering a double value.

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

This patch looks good except for the trailing white spaces.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +947,5 @@
>  
> +var uceFault_todouble_value = eval(uneval(uceFault).replace('uceFault', 'uceFault_todouble_value'))
> +function rtodouble_value(i) {
> +    var inputs = [ {}, [], 1, true, undefined, function(){}, null ];
> +    var expected = [ false, true, true, true, false, false, true];

nit: remove these two variables.

@@ +963,5 @@
> +}
> +
> +var uceFault_todouble_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_todouble_number'));
> +function rtodouble_number(i) {
> +    var x = Math.fround(Math.fround(i) + Math.fround(i)) + 1;    

nit: remove trailing white space.

::: js/src/jit/MIR.h
@@ +4311,5 @@
> +        if (input()->type() == MIRType_Value) 
> +            return false;
> +        if (input()->type() == MIRType_Symbol) 
> +            return false;
> +        

nit: remove trailing white space.

Comment 6

[Peter]

Attachment: Bug_1062888_5.patch

This is Bug_1062888.patch with the suggested edits (i.e. removal of trailing whitespaces and deletion of unused variables).

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8515614 [details] [diff] [review]
Bug_1062888_5.patch

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

This looks good, I will push this patch to our try server, such as we can verify that it wprks well before landing on mozilla-inbound.

Comment 8

[Peter]

Attachment: Formal hg patch (carying = nbp)

real `patch` from HG instead of a simple `hg diff > bidule.patch` like previous patches. sorry for that.

Comment 9

[Nicolas B. Pierron [:nbp]]

I pushed the patch on our try server.  The following link highlight the builds made by our build farm, which is also used as a continuous integration infrastructure:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=307c6fa1d3c6

Comment 10

[Nicolas B. Pierron [:nbp]]

I had to replace the RootedValue, by a Value, as apparently useless Rooted are scaring our static analysis.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd93434ea0b1

Sfink, apparently the issue from the previous try-push is caused by the function js::jit::RToDouble::recover.

Comment 11

[Nicolas B. Pierron [:nbp]]

Congratulation Peter, your first patch made it into mozilla-inbound \o/

You can see your patch as well as others on our continuous integration server:
https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound

Later today, these patches would be merged in mozilla-central, and they would be used to build Firefox Nightly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3789bf7f0e60

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3789bf7f0e60

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> I had to replace the RootedValue, by a Value, as apparently useless Rooted
> are scaring our static analysis.
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd93434ea0b1
> 
> Sfink, apparently the issue from the previous try-push is caused by the
> function js::jit::RToDouble::recover.

No, I suspect that code difference is irrelevant. The two failures on that push are different. I think they're both intermittents, but intermittent failures on the haz builds seem to be getting more and more frequent. I filed bug 1093355 for these. The hazard analysis uses up a bunch of memory during compilation, and I think it's pushing things over the edge.

Comment 14

[Peter]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Congratulation Peter, your first patch made it into mozilla-inbound \o/
> 
> You can see your patch as well as others on our continuous integration
> server:
> https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound
> 
> Later today, these patches would be merged in mozilla-central, and they
> would be used to build Firefox Nightly.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3789bf7f0e60

Thank you Nicolas. You own a big part of the patch but I am happy I could help.
Next patch is on its way :)