IonMonkey: Implement RToDouble Recover Instruction

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nbp, Assigned: peter, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Implement RToDouble in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
(Reporter)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Updated

5 years ago
Assignee: nobody → hv1989
(Reporter)

Updated

5 years ago
Assignee: hv1989 → nobody
Whiteboard: [good first bug][lang=c++]

Comment 1

5 years ago
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

5 years ago
(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.
(Reporter)

Comment 3

5 years ago
(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
(Reporter)

Updated

4 years ago
Blocks: 1079384
(Assignee)

Comment 4

4 years ago
Created attachment 8515539 [details] [diff] [review]
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
Attachment #8515539 - Flags: review?(nicolas.b.pierron)
(Reporter)

Updated

4 years ago
Assignee: nobody → peter
Status: NEW → ASSIGNED
(Reporter)

Comment 5

4 years ago
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.
Attachment #8515539 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8515614 [details] [diff] [review]
Bug_1062888_5.patch

This is Bug_1062888.patch with the suggested edits (i.e. removal of trailing whitespaces and deletion of unused variables).
Attachment #8515614 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 7

4 years ago
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.
Attachment #8515614 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Updated

4 years ago
Attachment #8515539 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8515620 [details] [diff] [review]
Formal hg patch (carying = nbp)

real `patch` from HG instead of a simple `hg diff > bidule.patch` like previous patches. sorry for that.
Attachment #8515614 - Attachment is obsolete: true
(Reporter)

Comment 9

4 years ago
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
(Reporter)

Comment 10

4 years ago
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.
Flags: needinfo?(sphink)
(Reporter)

Comment 11

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/3789bf7f0e60
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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.
Flags: needinfo?(sphink)
(Assignee)

Comment 14

4 years ago
(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 :)
You need to log in before you can comment on or make changes to this bug.