code review 6621061: cmd/5g, cmd/6g, cmd/8g: fix out of registers.

cmd/5g, cmd/6g, cmd/8g: fix out of registers.

This patch is enough to fix compilation of
exp/types tests but only passes a stripped down
version of the appripriate torture test.

Update issue 4207.

[remyoudompheng, 2012-10-08 21:18:36]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2012-10-08 21:20:09]

The ARM changes are totally untested. I would like something to try all.bash
with this patch on an ARM box.

torture.go was stripped down compared to what I would like to have but since the
compiler changes can be error-prone, I prefer keeping the changes small.

[remyoudompheng, 2012-10-08 21:34:30]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-10-08 23:30:59]

On 2012/10/08 21:34:30, remyoudompheng wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Strange results on arm, ./all.bash fails in dist bootstrap

panic: can't install method/function "len" with 462409278 results

goroutine 1 [running]:
text/template.addValueFuncs(0x1083b160, 0x1083b180, 0x0)
	/home/dfc/go/src/pkg/text/template/funcs.go:57 +0x420
text/template.createValueFuncs(0x1083b180, 0x1083b180, 0x1f93b0)
	/home/dfc/go/src/pkg/text/template/funcs.go:45 +0x50
text/template.init()
	/home/dfc/go/src/pkg/text/template/template.go:-484 +0x698
go/doc.init()
	/home/dfc/go/src/pkg/go/doc/synopsis.go:73 +0x88
main.init()
	/home/dfc/go/src/cmd/go/vet.go:37 +0x7c

goroutine 2 [runnable]:
created by runtime.main
	/home/dfc/go/src/pkg/runtime/proc.c:224

[remyoudompheng, 2012-10-08 23:37:53]

On 2012/10/08 23:30:59, dfc wrote:
> On 2012/10/08 21:34:30, remyoudompheng wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > Please take another look.
> 
> Strange results on arm, ./all.bash fails in dist bootstrap

There was some obvious garbage. I've added some bits from 6590046 and narrowed
the patch a bit so that it still compiles the test. Can you try patch set 5?

[dave_cheney.net, 2012-10-09 00:26:07]

Patchset #5 compiles and runs all tests without issue.

Thank you very much.

On Tue, Oct 9, 2012 at 10:37 AM,  <remyoudompheng@gmail.com> wrote:
> On 2012/10/08 23:30:59, dfc wrote:
>>
>> On 2012/10/08 21:34:30, remyoudompheng wrote:
>> > Hello mailto:golang-dev@googlegroups.com (cc:
>> mailto:golang-dev@googlegroups.com),
>> >
>> > Please take another look.
>
>
>> Strange results on arm, ./all.bash fails in dist bootstrap
>
>
> There was some obvious garbage. I've added some bits from 6590046 and
> narrowed the patch a bit so that it still compiles the test. Can you try
> patch set 5?
>
> https://codereview.appspot.com/6621061/

[dave_cheney.net, 2012-10-09 02:11:19]

benchmarks from linux/arm, I haven't tested other arches.

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   51788116000  53038696000   +2.41%
BenchmarkFannkuch11     33518280000  35096741000   +4.71%
BenchmarkGobDecode        128384400    119604500   -6.84%
BenchmarkGobEncode         57431020     67312620  +17.21%
BenchmarkGzip            5442810000   6653229000  +22.24%
BenchmarkGunzip          1098785000   1191376000   +8.43%
BenchmarkJSONEncode       832885800    866174200   +4.00%
BenchmarkJSONDecode      1750580000   1857239000   +6.09%
BenchmarkMandelbrot200     45679320     45626220   -0.12%
BenchmarkParse             60618900     60951540   +0.55%
BenchmarkRevcomp          132739250    138967800   +4.69%
BenchmarkTemplate        1995697000   2006500000   +0.54%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode             5.98         6.42    1.07x
BenchmarkGobEncode            13.36        11.40    0.85x
BenchmarkGzip                  3.57         2.92    0.82x
BenchmarkGunzip               17.66        16.29    0.92x
BenchmarkJSONEncode            2.33         2.24    0.96x
BenchmarkJSONDecode            1.11         1.04    0.94x
BenchmarkParse                 0.96         0.95    0.99x
BenchmarkRevcomp              19.15        18.29    0.96x
BenchmarkTemplate              0.97         0.97    1.00x

[dave_cheney.net, 2012-10-09 02:15:17]

Don't read too much into these benchmarks, rerunning them produces

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   52032105000  52645691000   +1.18%
BenchmarkFannkuch11     33484314000  35196441000   +5.11%
BenchmarkGobDecode        126373300    113630650  -10.08%
BenchmarkGobEncode         58969120     68316040  +15.85%
BenchmarkGzip            5445495000   6332855000  +16.30%
BenchmarkGunzip          1098053000   1193909000   +8.73%
BenchmarkJSONEncode       825732400    865692200   +4.84%
BenchmarkJSONDecode      1742492000   1855316000   +6.47%
BenchmarkMandelbrot200     45651260     45652480   +0.00%
BenchmarkParse             60066520     60984500   +1.53%
BenchmarkRevcomp          121865900    135211200  +10.95%
BenchmarkTemplate        1994445000   2005585000   +0.56%

[dave_cheney.net, 2012-10-10 03:26:16]

With this patch applied:

--- prog list "other" ---
0000 (/home/dfc/src/other.go:5) TEXT    other+0(SB),$0-0
0001 (/home/dfc/src/other.go:6) MOVW    $1,R2
0002 (/home/dfc/src/other.go:7) MOVW    $buf+0(SB),R0
0003 (/home/dfc/src/other.go:7) MOVW    $0(R0),R1
0004 (/home/dfc/src/other.go:7) MOVW    $32,R3
0005 (/home/dfc/src/other.go:7) CMP     R3,R2,
0006 (/home/dfc/src/other.go:7) BLO     ,8(APC)
0007 (/home/dfc/src/other.go:7) BL      ,runtime.panicindex+0(SB)
0008 (/home/dfc/src/other.go:7) ADD     R2,R1,R0
0009 (/home/dfc/src/other.go:7) MOVW    $1,R1
0010 (/home/dfc/src/other.go:7) MOVBU   R1,R1
0011 (/home/dfc/src/other.go:7) MOVBU   R1,0(R0)
0012 (/home/dfc/src/other.go:8) RET     ,
pando(~/src) % go build -gcflags -S other.go 
# command-line-arguments

Without this patch.

--- prog list "other" ---
0000 (/home/dfc/src/other.go:5) TEXT    other+0(SB),$0-0
0001 (/home/dfc/src/other.go:6) MOVW    $1,R1
0002 (/home/dfc/src/other.go:7) MOVW    $buf+0(SB),R0
0003 (/home/dfc/src/other.go:7) MOVW    $32,R2
0004 (/home/dfc/src/other.go:7) CMP     R2,R1,
0005 (/home/dfc/src/other.go:7) BLO     ,7(APC)
0006 (/home/dfc/src/other.go:7) BL      ,runtime.panicindex+0(SB)
0007 (/home/dfc/src/other.go:7) ADD     R1,R0
0008 (/home/dfc/src/other.go:7) MOVW    $1,R1
0009 (/home/dfc/src/other.go:7) MOVBU   R1,R1
0010 (/home/dfc/src/other.go:7) MOVBU   R1,0(R0)
0011 (/home/dfc/src/other.go:8) RET     ,

This probably explains why some of the benchmarks got slower.

[remyoudompheng, 2012-10-10 06:15:55]

On 2012/10/10 03:26:16, dfc wrote:
> This probably explains why some of the benchmarks got slower.

Probably. I suspect that the changes could affect also the number of temporaries
pinned to the stack. I would say it can improve a lot by adding the ONAME case
to igen, using igen in cgen and importing the componentgen function.

[remyoudompheng, 2012-10-10 18:34:10]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2012-10-10 18:36:20]

Dave (or anybody else), can you try and benchmark CL6639058:
http://codereview.appspot.com/6639058 

It adds optimizations on top on this CL and I would try to know if it addresses
the slowdowns. If it does, then the ARM part of this patch is not a problem and
the optimization can be submitted in a second step.

[dave_cheney.net, 2012-10-10 23:49:54]

On 2012/10/10 18:36:20, remyoudompheng wrote:
> Dave (or anybody else), can you try and benchmark CL6639058:
> http://codereview.appspot.com/6639058 
> 
> It adds optimizations on top on this CL and I would try to know if it
addresses
> the slowdowns. If it does, then the ARM part of this patch is not a problem
and
> the optimization can be submitted in a second step.

Hi Remy,

A substantial improvement here 

pando(~/go/test/bench/go1) % ./go1.1676e21191aa -test.bench=. > old.txt &&
./go1.cl6639058 -test.bench=. > new.txt && ~/go/misc/benchcmp {old,new}.txt
testing: warning: no tests to run
testing: warning: no tests to run
benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   51573303000  51879882000   +0.59%
BenchmarkFannkuch11     34040069000  33089233000   -2.79%
BenchmarkGobDecode        126087950    117199700   -7.05%
BenchmarkGobEncode         59175420     54542860   -7.83%
BenchmarkGzip            5396637000   5428344000   +0.59%
BenchmarkGunzip          1100036000   1060791000   -3.57%
BenchmarkJSONEncode       842144800    736047400  -12.60%
BenchmarkJSONDecode      1739960000   1698578000   -2.38%
BenchmarkMandelbrot200     45767200     45865480   +0.21%
BenchmarkParse             60036620     59382940   -1.09%
BenchmarkRevcomp          134698500    133169600   -1.14%
BenchmarkTemplate        1999207000   1849915000   -7.47%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode             6.09         6.55    1.08x
BenchmarkGobEncode            12.97        14.07    1.08x
BenchmarkGzip                  3.60         3.57    0.99x
BenchmarkGunzip               17.64        18.29    1.04x
BenchmarkJSONEncode            2.30         2.64    1.15x
BenchmarkJSONDecode            1.12         1.14    1.02x
BenchmarkParse                 0.96         0.98    1.02x
BenchmarkRevcomp              18.87        19.09    1.01x
BenchmarkTemplate              0.97         1.05    1.08x

[remyoudompheng, 2012-10-11 21:15:58]

Any other opinion on this?

[dave_cheney.net, 2012-10-11 23:38:13]

LGTM for 5g.

On Fri, Oct 12, 2012 at 8:15 AM,  <remyoudompheng@gmail.com> wrote:
> Any other opinion on this?
>
> http://codereview.appspot.com/6621061/

[nigeltao, 2012-10-12 07:07:36]

https://codereview.appspot.com/6621061/diff/14002/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

https://codereview.appspot.com/6621061/diff/14002/src/cmd/6g/cgen.c#newcode856
src/cmd/6g/cgen.c:856: regalloc(a, types[tptr], &n1);
Was the final argument supposed to change from res to &n1? Similarly in 5g/8g.

[remyoudompheng, 2012-10-12 21:25:20]

Hello dave@cheney.net, nigeltao@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2012-10-12 21:26:07]

http://codereview.appspot.com/6621061/diff/14002/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

http://codereview.appspot.com/6621061/diff/14002/src/cmd/6g/cgen.c#newcode856
src/cmd/6g/cgen.c:856: regalloc(a, types[tptr], &n1);
On 2012/10/12 07:07:36, nigeltao wrote:
> Was the final argument supposed to change from res to &n1? Similarly in 5g/8g.

Done.

[nigeltao, 2012-10-15 06:43:50]

LGTM.

[remyoudompheng, 2012-10-15 21:10:18]

I'll submit this in a few hours if there is no additional objection.

[remyoudompheng, 2012-10-16 05:22:40]

*** Submitted as http://code.google.com/p/go/source/detail?r=6b67a925abcc ***

cmd/5g, cmd/6g, cmd/8g: fix out of registers.

This patch is enough to fix compilation of
exp/types tests but only passes a stripped down
version of the appripriate torture test.

Update issue 4207.

R=dave, nigeltao, rsc, golang-dev
CC=golang-dev
http://codereview.appspot.com/6621061