code review 6494107: cmd/6g, cmd/8g: add OINDREG, ODOT, ODOTPTR cases to igen.

cmd/6g, cmd/8g: add OINDREG, ODOT, ODOTPTR cases to igen.

Apart from reducing the number of LEAL/LEAQ instructions by about
30%, it gives 8g easier registerization in several cases,
for example in strconv. Performance with 6g is not affected.

Before (386):
src/pkg/strconv/decimal.go:22   TEXT  (*decimal).String+0(SB),$240-12
src/pkg/strconv/extfloat.go:540 TEXT  (*extFloat).ShortestDecimal+0(SB),$584-20

After (386):
src/pkg/strconv/decimal.go:22   TEXT  (*decimal).String+0(SB),$196-12
src/pkg/strconv/extfloat.go:540 TEXT  (*extFloat).ShortestDecimal+0(SB),$420-20

Benchmarks with GOARCH=386 (on a Core 2).

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    7110191000   7079644000   -0.43%
BenchmarkFannkuch11      7769274000   7766514000   -0.04%
BenchmarkGobDecode         33454820     34755400   +3.89%
BenchmarkGobEncode         11675710     11007050   -5.73%
BenchmarkGzip            2013519000   1593855000  -20.84%
BenchmarkGunzip           253368200    242667600   -4.22%
BenchmarkJSONEncode       152443900    120763400  -20.78%
BenchmarkJSONDecode       304112800    247461800  -18.63%
BenchmarkMandelbrot200     29245520     29240490   -0.02%
BenchmarkParse              8484105      8088660   -4.66%
BenchmarkRevcomp         2695688000   2841263000   +5.40%
BenchmarkTemplate         363759800    277271200  -23.78%

benchmark                       old ns/op    new ns/op    delta
BenchmarkAtof64Decimal                127          129   +1.57%
BenchmarkAtof64Float                  166          164   -1.20%
BenchmarkAtof64FloatExp               308          300   -2.60%
BenchmarkAtof64Big                    584          571   -2.23%
BenchmarkAppendFloatDecimal           440          430   -2.27%
BenchmarkAppendFloat                  995          776  -22.01%
BenchmarkAppendFloatExp               897          746  -16.83%
BenchmarkAppendFloatNegExp            900          752  -16.44%
BenchmarkAppendFloatBig              1528         1228  -19.63%
BenchmarkAppendFloat32Integer         443          453   +2.26%
BenchmarkAppendFloat32ExactFraction   812          661  -18.60%
BenchmarkAppendFloat32Point          1002          773  -22.85%
BenchmarkAppendFloat32Exp             858          725  -15.50%
BenchmarkAppendFloat32NegExp          848          728  -14.15%
BenchmarkAppendFloat64Fixed1          447          431   -3.58%
BenchmarkAppendFloat64Fixed2          480          462   -3.75%
BenchmarkAppendFloat64Fixed3          461          457   -0.87%
BenchmarkAppendFloat64Fixed4          509          484   -4.91%

Update issue 1914.

[remyoudompheng, 2012-09-09 08:02:13]

Hello rsc@golang.org, golang-dev@googlegroups.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

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

[nigeltao, 2012-09-10 06:47:42]

FWIW, for 6g, the before/after for
go build -a -gcflags '-S' cmd/go | grep LEAQ | wc -l
is
40440 / 28647

The test/bench/go1 numbers are inconclusive:
benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    5949074000   5943610000   -0.09%
BenchmarkFannkuch11      3858978000   3946991000   +2.28%
BenchmarkGobDecode         20968930     21284030   +1.50%
BenchmarkGobEncode         12633550     12684320   +0.40%
BenchmarkGzip             592840200    583004600   -1.66%
BenchmarkGunzip           163234400    161619100   -0.99%
BenchmarkJSONEncode        88589500     89854700   +1.43%
BenchmarkJSONDecode       392493200    389656000   -0.72%
BenchmarkMandelbrot200      7016548      7017086   +0.01%
BenchmarkParse              7779295      7711245   -0.87%
BenchmarkRevcomp         1181691000   1121136000   -5.12%
BenchmarkTemplate         310984200    315470600   +1.44%

[remyoudompheng, 2012-09-10 06:56:58]

I think the patch is missing a refcount update on the register. It doesn't cause
an error (regfree: register not allocated), because I think the only cases that
happen have the form n(SP).
I'll have a look later.

[rsc, 2012-09-11 03:25:46]

yes, you should need a refcount update

[remyoudompheng, 2012-09-11 19:32:36]

Hello rsc@golang.org, golang-dev@googlegroups.com, nigeltao@golang.org (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-09-11 19:35:19]

I have added ODOT and ODOTPTR which were initially intended for a separate
patch.

I had to ignore D_AL in tests for registers left allocated in 8g. The thing is,
if I increment the reference count of a PARAM node, its register is
"uninitialised" to D_AL (in both 6g and 8g), which makes no sense but I suppose
means it is ignored.

We don't seem to use half registers in 8g so I assumed it was safe to simply
ignore it.

[rsc, 2012-09-17 21:10:24]

Looks good except for the loops in gsubr.c.
I think they should stay D_AL because you do care about smaller registers being
allocated, don't you?
Also there are other loops in the file that begin at D_AL that are not being
changed here. I would have expected them to be consistent.

http://codereview.appspot.com/6494107/diff/10001/src/cmd/8g/gsubr.c
File src/cmd/8g/gsubr.c (right):

http://codereview.appspot.com/6494107/diff/10001/src/cmd/8g/gsubr.c#newcode785
src/cmd/8g/gsubr.c:785: for(i=D_AX; i<=D_DI; i++)
Why did this change?

http://codereview.appspot.com/6494107/diff/10001/src/cmd/8g/gsubr.c#newcode795
src/cmd/8g/gsubr.c:795: for(i=D_AX; i<=D_DI; i++) {
Why did this change?

[remyoudompheng, 2012-09-17 21:21:32]

On 2012/09/17 21:10:24, rsc wrote:
> Looks good except for the loops in gsubr.c.
> I think they should stay D_AL because you do care about smaller registers
being
> allocated, don't you?

In my comment I explain it is an ugly workaround because function parameters,
although they are on the stack, have reg = D_AL. Maybe the correct fix is to
have them have D_SP instead?

I don't see where AL/AH/BL/BH can be used in 8g. In 6g they are not.

[rsc, 2012-09-17 21:26:02]

I think you are probably right that they cannot be used at all. In
that case let's change all the loops to start at D_AX instead of just
a few.

[remyoudompheng, 2012-09-17 23:25:34]

Hello rsc@golang.org, nigeltao@golang.org (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-09-17 23:27:35]

I apparently was confused by the n->type->type thing and added a comment to make
more explicit what type->type is.

[remyoudompheng, 2012-09-21 21:51:57]

ping

[rsc, 2012-09-24 14:38:38]

LGTM

Please double-check that I didn't get the TMAP parts backward.

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h#newcode173
src/cmd/gc/go.h:173: Type*	type;		// the element type, like Elem in reflect
// actual type for TFIELD, element type for TARRAY, TCHAN, TMAP, TPTRxx

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h#newcode177
src/cmd/gc/go.h:177: Type*	down;		// also used in TMAP
// also key type in TMAP

[remyoudompheng, 2012-09-24 20:15:08]

I will regenerate the benchmarks because some other patches were committed.

[remyoudompheng, 2012-09-24 21:09:17]

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

cmd/6g, cmd/8g: add OINDREG, ODOT, ODOTPTR cases to igen.

Apart from reducing the number of LEAL/LEAQ instructions by about
30%, it gives 8g easier registerization in several cases,
for example in strconv. Performance with 6g is not affected.

Before (386):
src/pkg/strconv/decimal.go:22   TEXT  (*decimal).String+0(SB),$240-12
src/pkg/strconv/extfloat.go:540 TEXT  (*extFloat).ShortestDecimal+0(SB),$584-20

After (386):
src/pkg/strconv/decimal.go:22   TEXT  (*decimal).String+0(SB),$196-12
src/pkg/strconv/extfloat.go:540 TEXT  (*extFloat).ShortestDecimal+0(SB),$420-20

Benchmarks with GOARCH=386 (on a Core 2).

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    7110191000   7079644000   -0.43%
BenchmarkFannkuch11      7769274000   7766514000   -0.04%
BenchmarkGobDecode         33454820     34755400   +3.89%
BenchmarkGobEncode         11675710     11007050   -5.73%
BenchmarkGzip            2013519000   1593855000  -20.84%
BenchmarkGunzip           253368200    242667600   -4.22%
BenchmarkJSONEncode       152443900    120763400  -20.78%
BenchmarkJSONDecode       304112800    247461800  -18.63%
BenchmarkMandelbrot200     29245520     29240490   -0.02%
BenchmarkParse              8484105      8088660   -4.66%
BenchmarkRevcomp         2695688000   2841263000   +5.40%
BenchmarkTemplate         363759800    277271200  -23.78%

benchmark                       old ns/op    new ns/op    delta
BenchmarkAtof64Decimal                127          129   +1.57%
BenchmarkAtof64Float                  166          164   -1.20%
BenchmarkAtof64FloatExp               308          300   -2.60%
BenchmarkAtof64Big                    584          571   -2.23%
BenchmarkAppendFloatDecimal           440          430   -2.27%
BenchmarkAppendFloat                  995          776  -22.01%
BenchmarkAppendFloatExp               897          746  -16.83%
BenchmarkAppendFloatNegExp            900          752  -16.44%
BenchmarkAppendFloatBig              1528         1228  -19.63%
BenchmarkAppendFloat32Integer         443          453   +2.26%
BenchmarkAppendFloat32ExactFraction   812          661  -18.60%
BenchmarkAppendFloat32Point          1002          773  -22.85%
BenchmarkAppendFloat32Exp             858          725  -15.50%
BenchmarkAppendFloat32NegExp          848          728  -14.15%
BenchmarkAppendFloat64Fixed1          447          431   -3.58%
BenchmarkAppendFloat64Fixed2          480          462   -3.75%
BenchmarkAppendFloat64Fixed3          461          457   -0.87%
BenchmarkAppendFloat64Fixed4          509          484   -4.91%

Update issue 1914.

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

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h#newcode173
src/cmd/gc/go.h:173: Type*	type;		// the element type, like Elem in reflect
On 2012/09/24 14:38:38, rsc wrote:
> // actual type for TFIELD, element type for TARRAY, TCHAN, TMAP, TPTRxx

Done.

http://codereview.appspot.com/6494107/diff/14001/src/cmd/gc/go.h#newcode177
src/cmd/gc/go.h:177: Type*	down;		// also used in TMAP
On 2012/09/24 14:38:38, rsc wrote:
> // also key type in TMAP

Done.