code review 6710043: cmd/5g: introduce componentgen for better registerization.

cmd/5g: introduce componentgen for better registerization.

It is essentially identical to the version in 6g.

[remyoudompheng, 2012-10-16 06:12:27]

Please test.

[dave_cheney.net, 2012-10-16 06:49:20]

On 2012/10/16 06:12:27, remyoudompheng wrote:
> Please test.

cmd/go
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0xe pc=0x146670]

goroutine 1 [running]:
regexp/syntax.Parse(0x1df30c, 0xbb, 0x2d00d4, 0x2, 0x2, ...)
        /home/dfc/go/src/pkg/regexp/syntax/parse.go:750 +0x168
regexp.compile(0x1df30c, 0xbb, 0xd4, 0xffffffff, 0x603c4, ...)
        /home/dfc/go/src/pkg/regexp/regexp.go:134 +0x38
regexp.Compile(0x1df30c, 0xbb, 0x0, 0xb6efce78)
        /home/dfc/go/src/pkg/regexp/regexp.go:107 +0x4c
regexp.MustCompile(0x1df30c, 0xbb, 0x1, 0x1)
        /home/dfc/go/src/pkg/regexp/regexp.go:195 +0x30
go/doc.init()
        /home/dfc/go/src/pkg/go/doc/synopsis.go:-1866 +0xb0
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

[dave_cheney.net, 2012-10-16 07:18:16]

On 2012/10/16 06:49:20, dfc wrote:
> On 2012/10/16 06:12:27, remyoudompheng wrote:
> > Please test.
> 
> cmd/go
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0xe pc=0x146670]
> 
> goroutine 1 [running]:
> regexp/syntax.Parse(0x1df30c, 0xbb, 0x2d00d4, 0x2, 0x2, ...)
>         /home/dfc/go/src/pkg/regexp/syntax/parse.go:750 +0x168
> regexp.compile(0x1df30c, 0xbb, 0xd4, 0xffffffff, 0x603c4, ...)
>         /home/dfc/go/src/pkg/regexp/regexp.go:134 +0x38
> regexp.Compile(0x1df30c, 0xbb, 0x0, 0xb6efce78)
>         /home/dfc/go/src/pkg/regexp/regexp.go:107 +0x4c
> regexp.MustCompile(0x1df30c, 0xbb, 0x1, 0x1)
>         /home/dfc/go/src/pkg/regexp/regexp.go:195 +0x30
> go/doc.init()
>         /home/dfc/go/src/pkg/go/doc/synopsis.go:-1866 +0xb0
> 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

comparing componentgen from the previous CL, this stands out as being removed


@@ -1502,7 +1501,7 @@
 int
 componentgen(Node *nr, Node *nl)
 {
-       Node nodl, nodr, tmp;
+       Node nodl, nodr;
        int freel, freer;
 
        freel = 0;
@@ -1534,12 +1533,6 @@
                        igen(nr, &nodr, N);
                        freer = 1;
                }
-       } else {
-               // When zeroing, prepare a register containing zero.
-               nodconst(&tmp, nl->type, 0);
-               regalloc(&nodr, types[TUINT], N);
-               gmove(&tmp, &nodr);
-               freer = 1;
        }

[remyoudompheng, 2012-10-17 20:23:34]

It seems to give different results when compiling with -N (tried running the
produced go tool on a friends's Raspberry).

I think I can say the bug is in the optimizer (regopt), but not in the peephole
optimizations. I see blocks of code disappearing mysteriously, and it only
happens when the componentgen change you mention is used [also this patch uses
componentgen in clearfat, which it wasn't doing before]

[remyoudompheng, 2012-10-17 21:07:57]

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

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

[remyoudompheng, 2012-10-17 21:09:01]

The bug was that mkvar was ignoring strings and slices (unlike 6g/8g) so it
wasn't treated as an outvar and they fell into "set but not used" and discarded.

Can you try again?

[dave_cheney.net, 2012-10-17 21:14:43]

Will do. Thank you
On 18 Oct 2012 08:09, <remyoudompheng@gmail.com> wrote:

> The bug was that mkvar was ignoring strings and slices (unlike 6g/8g) so
> it wasn't treated as an outvar and they fell into "set but not used" and
> discarded.
>
> Can you try again?
>
>
http://codereview.appspot.com/**6710043/<http://codereview.appspot.com/6710043/>
>

[dave_cheney.net, 2012-10-18 00:47:07]

LGTM.

Please note, the following benchmark has CL 6717043 (fast _div) applied in both
old and new cases. This is for two reasons, 1. it's got a LGTM, so will be
committed soon, 2. I believe without it, the effects of any improvements to
codegen are difficult to observe due to the increase of division operations in
the malloc codepath.

pando(~/go/test/bench/go1) % ./go1.5749e60d2b6b -test.bench=. > old.txt &&
./go1.test -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   45597656000  45800415000   +0.44%
BenchmarkFannkuch11     34541229000  33243561000   -3.76%
BenchmarkGobDecode        117576600    113871750   -3.15%
BenchmarkGobEncode         65834340     57223500  -13.08%
BenchmarkGzip            6355713000   5444794000  -14.33%
BenchmarkGunzip          1197907000   1063690000  -11.20%
BenchmarkJSONEncode       799755800    658972200  -17.60%
BenchmarkJSONDecode      1728577000   1597717000   -7.57%
BenchmarkMandelbrot200     45700680     45696420   -0.01%
BenchmarkParse             59507440     57044060   -4.14%
BenchmarkRevcomp          132467600    128094500   -3.30%
BenchmarkTemplate        1848328000   1707550000   -7.62%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode             6.53         6.74    1.03x
BenchmarkGobEncode            11.66        13.41    1.15x
BenchmarkGzip                  3.05         3.56    1.17x
BenchmarkGunzip               16.20        18.24    1.13x
BenchmarkJSONEncode            2.43         2.94    1.21x
BenchmarkJSONDecode            1.12         1.21    1.08x
BenchmarkParse                 0.97         1.02    1.05x
BenchmarkRevcomp              19.19        19.84    1.03x
BenchmarkTemplate              1.05         1.14    1.09x

[remyoudompheng, 2012-10-18 06:59:16]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2012-10-18 09:31:00]

http://codereview.appspot.com/6710043/diff/9001/src/cmd/5g/reg.c
File src/cmd/5g/reg.c (right):

http://codereview.appspot.com/6710043/diff/9001/src/cmd/5g/reg.c#newcode989
src/cmd/5g/reg.c:989: case TFUNC:
comparing to 6g, this looks correct.

[remyoudompheng, 2012-10-26 06:41:33]

ping
any opinion on where there is going?

[dave_cheney.net, 2012-10-26 13:55:04]

Still LGTM.

On Fri, Oct 26, 2012 at 5:41 PM,  <remyoudompheng@gmail.com> wrote:
> ping
> any opinion on where there is going?
>
> http://codereview.appspot.com/6710043/

[remyoudompheng, 2012-10-27 10:03:34]

I'll submit this tomorrow unless there are remarks.

[remyoudompheng, 2012-10-28 19:11:28]

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

cmd/5g: introduce componentgen for better registerization.

It is essentially identical to the version in 6g.

R=dave, minux.ma, rsc
CC=golang-dev
http://codereview.appspot.com/6710043

[minux1, 2012-12-18 23:32:52]

Though strange, I just determined that this change is responsible for a test
failure of Go on Darwin/ARM.

Without this change, fixedbugs/bug348.go could get this traceback (I just
printed out pc, file, line from bug348.go):
8488 /var/mobile/go/src/bug348.go 28
59468 /var/mobile/go/src/pkg/runtime/panic.c 134
61048 /var/mobile/go/src/pkg/runtime/panic.c 384
87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431
8428 /var/mobile/go/src/bug348.go 47
8232 /var/mobile/go/src/bug348.go 17

With this change, the traceback becomes:
8472 /var/mobile/go/src/bug348.go 28
59364 /var/mobile/go/src/pkg/runtime/panic.c 134
60944 /var/mobile/go/src/pkg/runtime/panic.c 384
86952 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431
8412 /var/mobile/go/src/bug348.go 47
8224 /var/mobile/go/src/bug348.go 15
BUG: bug348: panic at /var/mobile/go/src/bug348.go:15 in main.f

Remy, do you have any ideas why this change could affect the pc->line number
mapping?
Thank you.

The weird thing is that it doesn't affect Linux/ARM, only Darwin/ARM.
(The port is here: http://bitbucket.org/minux/goios/, it doesn't make changes to
5g)

[rsc, 2012-12-18 23:53:39]

Are you sync'ed past http://code.google.com/p/go/source/detail?r=0f491e663a44?

[minux1, 2012-12-19 00:04:25]

On Wednesday, December 19, 2012, Russ Cox wrote:

> Are you sync'ed past
> http://code.google.com/p/go/source/detail?r=0f491e663a44?
>
yes. i just merged default branch of today, noticed the problem,
and backtracked to this cl.

5g -S shows the line number is correct with this cl, but the only factor
changed is just 5g,
so i'm pretty confused.

[rsc, 2012-12-19 00:16:50]

Can you post the 5g -S output? I don't have a working 5g here.

[minux1, 2012-12-19 00:24:52]

On Wednesday, December 19, 2012, minux wrote:
>
> On Wednesday, December 19, 2012, Russ Cox wrote:
>
>> Are you sync'ed past
>> http://code.google.com/p/go/source/detail?r=0f491e663a44?
>>
> yes. i just merged default branch of today, noticed the problem,
> and backtracked to this cl.
>
> 5g -S shows the line number is correct with this cl, but the only factor
> changed is just 5g,
> so i'm pretty confused.
>
the pc -> line number conversion code seems to have a off-by-one error
only on darwin/arm.

bad.s
--- prog list "f" ---
0000 (bug348.go:14) TEXT        f+0(SB),$36-0
0001 (bug348.go:15) MOVW        $0,R0
0002 (bug348.go:17) MOVW        0(R0),R1. // segfault happens here
0003 (bug348.go:17) MOVW        R1,autotmp_0000+-16(SP)
0004 (bug348.go:17) MOVW        4(R0),R1
0005 (bug348.go:17) MOVW        R1,autotmp_0000+-12(SP)
0006 (bug348.go:17) MOVW        $0,R2
0007 (bug348.go:17) MOVW        R2,autotmp_0001+-8(SP)

good.s:
--- prog list "f" ---
0000 (bug348.go:14) TEXT        f+0(SB),$36-0
0001 (bug348.go:15) MOVW        $0,R2
0002 (bug348.go:17) MOVW        R2,R1
0003 (bug348.go:17) MOVW        $autotmp_0000+-16(SP),R0
0004 (bug348.go:17) MOVW.P      4(R1),R2.  // sigfault happens here
0005 (bug348.go:17) MOVW.P      R2,4(R0)
0006 (bug348.go:17) MOVW.P      4(R1),R2
0007 (bug348.go:17) MOVW.P      R2,4(R0)

i can confirm the code generated for linux/arm is the same, so this
cl only exposes the error in runtime.
however, i don't remember i touched pcln code.....

[rsc, 2012-12-19 00:40:14]

Normally the return PC for a call leads to the instruction after the
call, so the traceback code moves backward one instruction, in order
to print the line of the call instead of the line of the code that
follows the call. A signal like this is made to look like a call
returning to the signaling instruction, so that if it did get handled
and the signal handler returned, the instruction would be given a
second chance to execute. If the traceback code shows the line of the
previous instruction instead of the line of the actual instruction,
that would explain the behavior you are seeing.

runtime/traceback_arm.c says

                        if(runtime·showframe(f)) {
                                // Print during crash.
                                //      main(0x1, 0x2, 0x3)
                                //
/home/rsc/go/src/runtime/x.go:23 +0xf
                                tracepc = pc;   // back up to CALL
instruction for funcline.
                                if(n > 0 && pc > f->entry && !waspanic)
                                        tracepc -= sizeof(uintptr);

It sounds like maybe in your case waspanic is 0 here but should be 1.

waspanic is set during the previous frame, as waspanic = f->entry ==
(uintptr)runtime·sigpanic. That's the thread_darwin.c:431 frame have
on your stack, but it is strange that there is a bug348.go:47 frame
between thread_darwin.c:431 and bug348.go:15/17. Why does the panic
appear to have happened in main.main called from main.f, instead of
the other way around?

That is:
8488 /var/mobile/go/src/bug348.go 28
59468 /var/mobile/go/src/pkg/runtime/panic.c 134
61048 /var/mobile/go/src/pkg/runtime/panic.c 384
87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431
8428 /var/mobile/go/src/bug348.go 47 <<<
8232 /var/mobile/go/src/bug348.go 17 <<<

The two <<< lines seem backward to me. If you can arrange for them to
end up in the right order then I think everything will start working.
I expect the bug is in runtime.sighandler in your signal_darwin_arm.c.

Russ

[minux1, 2012-12-19 01:18:54]

thank you for your detailed explanation.

On Wednesday, December 19, 2012, Russ Cox wrote:

> Normally the return PC for a call leads to the instruction after the
> call, so the traceback code moves backward one instruction, in order
> to print the line of the call instead of the line of the code that
> follows the call. A signal like this is made to look like a call
> returning to the signaling instruction, so that if it did get handled
> and the signal handler returned, the instruction would be given a
> second chance to execute. If the traceback code shows the line of the
> previous instruction instead of the line of the actual instruction,
> that would explain the behavior you are seeing.
>
> runtime/traceback_arm.c says
>
>                         if(runtime·showframe(f)) {
>                                 // Print during crash.
>                                 //      main(0x1, 0x2, 0x3)
>                                 //
> /home/rsc/go/src/runtime/x.go:23 +0xf
>                                 tracepc = pc;   // back up to CALL
> instruction for funcline.
>                                 if(n > 0 && pc > f->entry && !waspanic)
>                                         tracepc -= sizeof(uintptr);
>
> It sounds like maybe in your case waspanic is 0 here but should be 1.
>
> waspanic is set during the previous frame, as waspanic = f->entry ==
> (uintptr)runtime·sigpanic. That's the thread_darwin.c:431 frame have
> on your stack, but it is strange that there is a bug348.go:47 frame
> between thread_darwin.c:431 and bug348.go:15/17. Why does the panic
> appear to have happened in main.main called from main.f, instead of
> the other way around?
>
> That is:
> 8488 /var/mobile/go/src/bug348.go 28
> 59468 /var/mobile/go/src/pkg/runtime/panic.c 134
> 61048 /var/mobile/go/src/pkg/runtime/panic.c 384
> 87056 /var/mobile/go/src/pkg/runtime/thread_darwin.c 431
> 8428 /var/mobile/go/src/bug348.go 47 <<<
> 8232 /var/mobile/go/src/bug348.go 17 <<<
>
> The two <<< lines seem backward to me. If you can arrange for them to
> end up in the right order then I think everything will start working.
> I expect the bug is in runtime.sighandler in your signal_darwin_arm.c.

i figure out the stupid error i made in the signal handler.
thank you russ for pointing me directly to the error.
thank you remy for exposing the error.

i will make a test to catch this kind of error in the future.