code review 6442114: cmd/6g, cmd/8g: eliminate short integer arithmetic when...

cmd/6g, cmd/8g: eliminate short integer arithmetic when possible.

Fixes issue 3909.
Fixes issue 3910.

[remyoudompheng, 2012-08-11 12:15:43]

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-08-11 12:22:46]

This patch intends to replace CL 6446088 and CL 6459046 by eliminating short
integer instructions whenever possible. It passes the tests but can cause more
issues: there were some with the carry bit (needc is not complete) and a use of
ORB with arguments being actual ints and not bytes (in the uint64 -> float64
conversion).

[nigeltao, 2012-08-13 07:36:26]

http://codereview.appspot.com/6442114/diff/7001/src/cmd/6g/peep.c
File src/cmd/6g/peep.c (right):

http://codereview.appspot.com/6442114/diff/7001/src/cmd/6g/peep.c#newcode139
src/cmd/6g/peep.c:139: // byte, word arithmetic elimination.
s/spaces/tabs/. Similarly on the next line.

http://codereview.appspot.com/6442114/diff/7001/src/cmd/8g/peep.c
File src/cmd/8g/peep.c (right):

http://codereview.appspot.com/6442114/diff/7001/src/cmd/8g/peep.c#newcode133
src/cmd/8g/peep.c:133: // byte, word arithmetic elimination.
s/spaces/tabs/.

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go
File test/fixedbugs/bug440_64.go (right):

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go#n...
test/fixedbugs/bug440_64.go:1: // run
Dumb question: is this file new? Why don't I see it in my tree?

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go#n...
test/fixedbugs/bug440_64.go:4: // about 64- vs 64-bit moves during
splitContractIndex.
"64- vs 64-bit" makes no sense.

[remyoudompheng, 2012-08-13 07:58:11]

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

Please take another look.

[remyoudompheng, 2012-08-13 07:59:23]

http://codereview.appspot.com/6442114/diff/7001/src/cmd/6g/peep.c
File src/cmd/6g/peep.c (right):

http://codereview.appspot.com/6442114/diff/7001/src/cmd/6g/peep.c#newcode139
src/cmd/6g/peep.c:139: // byte, word arithmetic elimination.
On 2012/08/13 07:36:26, nigeltao wrote:
> s/spaces/tabs/. Similarly on the next line.

Done.

http://codereview.appspot.com/6442114/diff/7001/src/cmd/8g/peep.c
File src/cmd/8g/peep.c (right):

http://codereview.appspot.com/6442114/diff/7001/src/cmd/8g/peep.c#newcode133
src/cmd/8g/peep.c:133: // byte, word arithmetic elimination.
On 2012/08/13 07:36:26, nigeltao wrote:
> s/spaces/tabs/.

Done.

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go
File test/fixedbugs/bug440_64.go (right):

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go#n...
test/fixedbugs/bug440_64.go:1: // run
The patch says "copy from test/bugs/bug440.go"

http://codereview.appspot.com/6442114/diff/7001/test/fixedbugs/bug440_64.go#n...
test/fixedbugs/bug440_64.go:4: // about 64- vs 64-bit moves during
splitContractIndex.
On 2012/08/13 07:36:26, nigeltao wrote:
> "64- vs 64-bit" makes no sense.

Done.

[rsc, 2012-08-31 11:37:17]

LGTM

Looks fantastic. Thank you.

http://codereview.appspot.com/6442114/diff/5006/src/cmd/6g/peep.c
File src/cmd/6g/peep.c (right):

http://codereview.appspot.com/6442114/diff/5006/src/cmd/6g/peep.c#newcode469
src/cmd/6g/peep.c:469: elimshortmov(Reg *r) {
{ on next line (sorry)

http://codereview.appspot.com/6442114/diff/5006/src/cmd/6g/peep.c#newcode494
src/cmd/6g/peep.c:494: // movb into register.
// move or arithmetic into partial register.

is multiply not uniform enough to be handled here too?

http://codereview.appspot.com/6442114/diff/5006/src/cmd/8g/peep.c
File src/cmd/8g/peep.c (right):

http://codereview.appspot.com/6442114/diff/5006/src/cmd/8g/peep.c#newcode313
src/cmd/8g/peep.c:313: elimshortmov(Reg *r) {
{ on next line

[remyoudompheng, 2012-09-01 10:41:25]

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

Please take another look.

[rsc, 2012-09-01 14:10:41]

LGTM

http://codereview.appspot.com/6442114/diff/15001/src/cmd/6g/peep.c
File src/cmd/6g/peep.c (right):

http://codereview.appspot.com/6442114/diff/15001/src/cmd/6g/peep.c#newcode499
src/cmd/6g/peep.c:499: // TODO: also consider eliminating 32-bit ops?
You can drop this TODO. The problem with the byte and word ops is that they are
defined to preserve the high bits of the destination register, meaning that even
though _we_ don't care, the processor must treat the destination as an
additional input where it might otherwise not be (especially for MOV, less so
for this arithmetic). That constrains register renaming which makes operations
less fast than they might otherwise be. The 32-bit ops do not have this problem
because they are defined to clear the top 32 bits of the destination register
always.

[remyoudompheng, 2012-09-01 14:41:02]

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

cmd/6g, cmd/8g: eliminate short integer arithmetic when possible.

Fixes issue 3909.
Fixes issue 3910.

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

http://codereview.appspot.com/6442114/diff/15001/src/cmd/6g/peep.c
File src/cmd/6g/peep.c (right):

http://codereview.appspot.com/6442114/diff/15001/src/cmd/6g/peep.c#newcode499
src/cmd/6g/peep.c:499: // TODO: also consider eliminating 32-bit ops?
On 2012/09/01 14:10:41, rsc wrote:
> You can drop this TODO. The problem with the byte and word ops is that they
are
> defined to preserve the high bits of the destination register, meaning that
even
> though _we_ don't care, the processor must treat the destination as an
> additional input where it might otherwise not be (especially for MOV, less so
> for this arithmetic). That constrains register renaming which makes operations
> less fast than they might otherwise be. The 32-bit ops do not have this
problem
> because they are defined to clear the top 32 bits of the destination register
> always.

Done.

[minux1, 2012-09-02 19:25:24]

one possible typo

http://codereview.appspot.com/6442114/diff/21002/src/cmd/8g/peep.c
File src/cmd/8g/peep.c (right):

http://codereview.appspot.com/6442114/diff/21002/src/cmd/8g/peep.c#newcode310
src/cmd/8g/peep.c:310: // can smash the entire 64-bit register without
64-bit register?