code review 6450163: cmd/8g: roll back the small integer constant optimizati...

cmd/8g: roll back the small integer constant optimizations introduced
in 13416:67c0b8c8fb29 "faster code, mainly for rotate" [1]. The codegen
can run out of registers if there are too many small-int arithmetic ops.

An alternative approach is to copy 6g's sbop/abop codegen to 8g, but
this change is less risky.

Fixes issue 3835.

[1] http://code.google.com/p/go/source/diff?spec=svn67c0b8c8fb29b1b7b6221977af6b89cae787b941&name=67c0b8c8fb29&r=67c0b8c8fb29b1b7b6221977af6b89cae787b941&format=side&path=/src/cmd/8g/cgen.c

[nigeltao, 2012-08-20 07:21:25]

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

I'd like you to review this change to
https://code.google.com/p/go

[remyoudompheng, 2012-08-22 21:13:19]

LGTM, can someone else have a look?

[rsc, 2012-08-22 21:27:32]

LGTM

This is fine if you reopen the bug after submitting so that we remember to
restore a working version of the code later.

Alternately, I think this might fix it now:

if(smallintconst(nr)) {
    tempname(&n1, nl->type);
    cgen(nl, &n1);
    regalloc(&n2, nl->type, res);
    gmove(&n1, &n2);
    gins(a, nr, &n2);
    regfree(&n2);
}

The crummy thing about the 386 is that you really can't ever have a register
allocated during a call to cgen.

[r, 2012-08-22 21:30:35]

Generating code AND allocating registers? Forsooth! Albert, get my gun.

-rob

[remyoudompheng, 2012-08-22 21:38:58]

On 2012/08/22 21:27:32, rsc wrote:
> Alternately, I think this might fix it now:
> 
> if(smallintconst(nr)) {
>     tempname(&n1, nl->type);
>     cgen(nl, &n1);
>     regalloc(&n2, nl->type, res);
>     gmove(&n1, &n2);
>     gins(a, nr, &n2);
>     regfree(&n2);
> }
>

It is what I was suggesting in CL 6450091 in an attempt to fix oother out of
registers situations.

[nigeltao, 2012-08-23 06:17:36]

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

cmd/8g: roll back the small integer constant optimizations introduced
in 13416:67c0b8c8fb29 "faster code, mainly for rotate" [1]. The codegen
can run out of registers if there are too many small-int arithmetic ops.

An alternative approach is to copy 6g's sbop/abop codegen to 8g, but
this change is less risky.

Fixes issue 3835.

[1]
http://code.google.com/p/go/source/diff?spec=svn67c0b8c8fb29b1b7b6221977af6b8...

R=rsc, remyoudompheng, r
CC=golang-dev
http://codereview.appspot.com/6450163

[nigeltao, 2012-08-23 07:19:15]

On 23 August 2012 07:27,  <rsc@golang.org> wrote:
> if(smallintconst(nr)) {
>     tempname(&n1, nl->type);
>     cgen(nl, &n1);
>     regalloc(&n2, nl->type, res);
>     gmove(&n1, &n2);
>     gins(a, nr, &n2);
>     regfree(&n2);
> }

This doesn't work, but I'm not thinking clearly enough to see why. For
this simple program:

--------
package main

var a, b, c int

func main() {
    c = a + 2 + b
}
--------

Your suggested patch generates this code:

--- prog list "main" ---
0000 (main.go:5) TEXT    main+0(SB),$4-0
0001 (main.go:6) MOVL    a+0(SB),AX
0002 (main.go:6) MOVL    AX,BX
0003 (main.go:6) ADDL    $2,BX
0004 (main.go:6) MOVL    autotmp_0000+-4(SP),BX
0005 (main.go:6) ADDL    b+0(SB),BX
0006 (main.go:6) MOVL    BX,c+0(SB)
0007 (main.go:7) RET     ,

Note that it's loading an uninitialized value from the stack. In
comparison, 8g at tip (given that the original CL that started this
mail thread has been submitted) generates:

--- prog list "main" ---
0000 (main.go:5) TEXT    main+0(SB),$0-0
0001 (main.go:6) MOVL    a+0(SB),AX
0002 (main.go:6) ADDL    $2,AX
0003 (main.go:6) MOVL    AX,BX
0004 (main.go:6) ADDL    b+0(SB),BX
0005 (main.go:6) MOVL    BX,c+0(SB)
0006 (main.go:7) RET     ,

This isn't perfect (the BX's could become AX's and the "MOVL AX, AX"
removed), but isn't obviously worse, and doesn't require any stack for
the tempname. For this trivial program, 8g (at tip) actually generates
the same code as it did prior to the original CL, with the broken
allocate-registers-upfront code.

I could spend some more time debugging your proposal, but I'm not sure
if the current codegen is so bad. Do you remember the original Go
expression that provoked the smallintconst change? Was it something in
md5/sha1?

[rsc, 2012-08-31 16:55:16]

It probably was md5/sha1. Don't worry much about it. I reopened the
bug so we don't forget but it's not very high priority.