Closed Bug 876607 Opened 7 years ago Closed 7 years ago

IonMonkey: Reordering of operands should look to real use count

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

1) Currently we base the reordering of operands on the useCount. This is actually a little bit incorrect, since that also includes MResumePoints. I don't think they should be counted.

2) Currently if rhs->useCount() == 1, we still reorder when lhs->useCount() is also 1. As a result we reoder z += a to z = a + z . I would suggest to keep the original order when useCount of both operands is 1.

Note: I looked to other places where we use useCount() instead of the newly introduced defUseCount(), but there are no other places where we definitely need to use the newer one.
Attached patch PatchSplinter Review
Assignee: general → hv1989
Attachment #754708 - Flags: review?(sstangl)
Blocks: 875135
Blocks: 876247
awfy-assorted:
 basic-fpops:                     219.0ms to 183.0ms
 bugs-608733-interpreter:         523.0ms to 505.0ms
 bugs-774119-cpg-arrays:          828.0m to 743.0ms
Something is wrong with the attachment: I don't have a review option, and I can't look at the patch outside of gedit. Could you check that the flags are in order, and maybe reupload?
Comment on attachment 754708 [details] [diff] [review]
Patch

Try again. :-)
Attachment #754708 - Attachment is patch: true
Attachment #754708 - Attachment mime type: text/x-patch → text/plain
Attachment #754708 - Flags: review?(sstangl) → review+
For bookkeeping this also removes a 7% improvement on kraken-oscillator. This was added by bug 870095, but seems like we accidentially reordered the operands, instead of based on real information. If we want to claw this back, we will need to make this switching more robust and e.g. incorporate information about loops
https://hg.mozilla.org/mozilla-central/rev/15738f337df2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
For the record, this also seems to have caused a 3-4% regression on awfy for asmjs-apps-bullet-workload[1-3]
Also, a 2.3% regression on asmjs-ubench-copy.
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ed321f3fbe

Fix messing up and switching the lhs/rhs definitions. Caused the asmjs regressions.
Target Milestone: mozilla24 → ---
And it also brings v8-crypto back to its old score. Cool! (My measurements didn't pick this up)
Blocks: 876057
You need to log in before you can comment on or make changes to this bug.