Closed Bug 903410 Opened 8 years ago Closed 8 years ago

Fix LSRA to eliminate some redundant stores

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
Marty and Doug mentioned a redundant store on ARM on the asm.js nsieve benchmark.

I wrote the attached patch to get rid of some redundant spills. dougc tested the patch and it doesn't get rid of this particular store, but at least on x86 it does eliminate spills on almost all benchmarks I tried, so I'd like to get this in.

On x86, the patch eliminates ~176 spill instructions on the asm.js fannkuch ubench and wins about 10%. There also seem to be small wins on other benchmarks.
Attachment #788123 - Flags: review?(bhackett1024)
Summary: Fix LSRA to eliminate some some redundant stores → Fix LSRA to eliminate some redundant stores
Attached patch Part 2Splinter Review
If a value is spilled at the loop header, don't spill it before the backedge.

This patch gets rid of the store Marty and Doug found on ARM with the nsieve benchmark. It also eliminates a spill on x86 in Octane-crypto am3 and a bunch of other spills on Kraken/Octane/SS.

Problem is, this optimization confuses the regalloc verifier. The patch fixes this by adding "hints" to the movegroup in debug builds, but it's a bit lame. Brian, can you think of a way to fix the verifier that's not so gross?

If it's a win somewhere we can take this patch but I'm a bit unhappy about the verifier workaround.
Attachment #788251 - Flags: feedback?(bhackett1024)
First patch gives me a 10% speedup on lua-binarytrees and lua-scimark, 5% speedup on zlib, and various other small speedups.

Second patch on top of that appears to give some small speedups, but within the range of noise so not sure, but the fact most of the small changes are in the right direction is a good indication.
(all on 64-bit linux)
Attachment #788123 - Flags: review?(bhackett1024) → review+
Comment on attachment 788251 [details] [diff] [review]
Part 2

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

How is the verifier getting confused here?  Can you post a testcase which triggers the verifier assert?  It seems like a bug in the verifier itself, the backwards flow analysis done there should be able to cope with vregs that are only spilled in the loop preheader.
https://hg.mozilla.org/mozilla-central/rev/6d3458473c20
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Attachment #788251 - Flags: feedback?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.