903410 - Fix LSRA to eliminate some redundant stores

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 2

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.

Comment 2

[Alon Zakai (:azakai)]

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.

Comment 3

[Alon Zakai (:azakai)]

(all on 64-bit linux)

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3458473c20

Comment 5

[Brian Hackett [Laid off!]]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d3458473c20