[golang-dev] Re: code review 6733055: cmd/5g, cmd/6g: fix out of registers with array indexing. (issue 6733055)

http://codereview.appspot.com/6733055/diff/14001/src/cmd/5g/gsubr.c
File src/cmd/5g/gsubr.c (right):

http://codereview.appspot.com/6733055/diff/14001/src/cmd/5g/gsubr.c#newcode1809
src/cmd/5g/gsubr.c:1809: // disabled.
could you please leave a small comment explaining why this is disabled.

http://codereview.appspot.com/6733055/diff/14001/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

http://codereview.appspot.com/6733055/diff/14001/src/cmd/6g/gsubr.c#newcode1953
src/cmd/6g/gsubr.c:1953: if(n->left->type->etype == TSTRING)
is this dead code ?

http://codereview.appspot.com/6733055/

linux/arm, omap4:

benchmark old ns/op new ns/op delta
BenchmarkBinaryTree17 46192261000 45795379000 -0.86%
BenchmarkFannkuch11 34446014000 34150849000 -0.86%
BenchmarkGobDecode 115428150 113720700 -1.48%
BenchmarkGobEncode 57655040 56268920 -2.40%
BenchmarkGzip 5620087000 5498596000 -2.16%
BenchmarkGunzip 1077423000 1008453000 -6.40%
BenchmarkJSONEncode 678076200 677569600 -0.07%
BenchmarkJSONDecode 1585084000 1570160000 -0.94%
BenchmarkMandelbrot200 45711060 45711680 +0.00%
BenchmarkParse 60339960 56372060 -6.58%
BenchmarkRevcomp 126818850 128361500 +1.22%
BenchmarkTemplate 1731140000 1724090000 -0.41%

benchmark old MB/s new MB/s speedup
BenchmarkGobDecode 6.65 6.75 1.02x
BenchmarkGobEncode 13.31 13.64 1.02x
BenchmarkGzip 3.45 3.53 1.02x
BenchmarkGunzip 18.01 19.24 1.07x
BenchmarkJSONEncode 2.86 2.86 1.00x
BenchmarkJSONDecode 1.22 1.24 1.02x
BenchmarkParse 0.96 1.03 1.07x
BenchmarkRevcomp 20.04 19.80 0.99x
BenchmarkTemplate 1.12 1.13 1.01x


http://codereview.appspot.com/6733055/

http://codereview.appspot.com/6733055/diff/14001/src/cmd/5g/cgen.c
File src/cmd/5g/cgen.c (right):

http://codereview.appspot.com/6733055/diff/14001/src/cmd/5g/cgen.c#newcode599
src/cmd/5g/cgen.c:599: USED(nr);
If nr is not used, can it be removed?

http://codereview.appspot.com/6733055/

Hello [email protected], [email protected] (cc:
[email protected]),

Please take another look.


http://codereview.appspot.com/6733055/

I'm surprised to see a noticeable performance improvement.

sudoaddable is essentially unused after this patch. Its role is
essentially to implement something similar to igen for OINDEX nodes (the
ODOT and ODOTPTR cases are redundant with igen), and allow 6g to
generate instructions like "MOVQ (R8)(8*R9), R10".

http://codereview.appspot.com/6733055/

Could this be an improvement in stack usage ? If this is truly
unexpected, I can investigate further, but I am happy that there is no
performance regression.
I am very happy to see sudoaddable on its way out the door.


http://codereview.appspot.com/6733055/

My immediate guess would be that postfix allocation of registers causes
some of the registers to be reused when they otherwise wouldn't. Makes
the job of regopt significantly easier.

https://codereview.appspot.com/6733055/

Here is an analysis of the difference in stack usage between
3312fddeb739 without and with this CL applied. From my reading, the
differences are all increases in stack usage.

There is no difference in escape analysis for the same sample, non was expected.

For clarity, this analysis was on linux/amd64 (not arm)

http://codereview.appspot.com/6733055/

On arm the difference is very small. I think the difference on amd64 is
just a manifestation of a deeper problem. See
http://codereview.appspot.com/6821052

http://codereview.appspot.com/6733055/