code review 5966075: cmd/gc: inline slice[arr,str] in the frontend (mostly).

cmd/gc: inline slice[arr,str] in the frontend (mostly).

[lvd, 2012-04-16 11:30:40]

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

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

[lvd, 2012-04-16 11:33:02]

6g and 8g pass all tests
for 5g i need to be able to test somewhere
/L

On Mon, Apr 16, 2012 at 13:30, <lvd@google.com> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/gc: inline slice[arr,str] in the frontend (mostly).
>
> Please review this at
http://codereview.appspot.com/**5966075/<http://codereview.appspot.com/5966075/>
>
> Affected files:
>  M src/cmd/5g/cgen.c
>  M src/cmd/5g/gg.h
>  M src/cmd/5g/ggen.c
>  M src/cmd/5g/gsubr.c
>  M src/cmd/6g/cgen.c
>  M src/cmd/6g/gg.h
>  M src/cmd/6g/ggen.c
>  M src/cmd/6g/gsubr.c
>  M src/cmd/8g/cgen.c
>  M src/cmd/8g/gg.h
>  M src/cmd/8g/ggen.c
>  M src/cmd/8g/gsubr.c
>  M src/cmd/gc/builtin.c
>  M src/cmd/gc/gen.c
>  M src/cmd/gc/go.h
>  M src/cmd/gc/runtime.go
>  M src/cmd/gc/walk.c
>  M src/pkg/runtime/slice.c
>  M src/pkg/runtime/string.goc
>
>
>

[ality, 2012-04-16 11:46:43]

This is glorious. Over 1000 lines deleted.

Do we have any slice heavy benchmarks to
see the effect it has?

  Anthony

[lvd, 2012-04-16 12:56:18]

On Mon, Apr 16, 2012 at 13:46, Anthony Martin <ality@pbrane.org> wrote:

> This is glorious. Over 1000 lines deleted.
>
> Do we have any slice heavy benchmarks to
> see the effect it has?
>

suggesions for realistic benchmarks are welcome.  the newly generated code
tends to have some spurious loads and stores that the registerizer doesn't
seem to catch, i'll look closer at that, but on the other hand, this now
handles /all OSLICExxx whereas the old code would leave some calls to
runtime.slicexxx in.

to get an idea here is the old/new 6g -S output for

func h(s []int, lo, hi int) []int {
        return s[lo:hi]
}

before:

0000 (sl.go:3) TEXT    h+0(SB),$0-40
0001 (sl.go:3) MOVL    s+12(FP),AX
0002 (sl.go:4) MOVL    lo+16(FP),BX
0003 (sl.go:4) MOVLQSX BX,BX
0004 (sl.go:4) MOVL    hi+20(FP),BP
0005 (sl.go:4) MOVLQSX BP,BP
0006 (sl.go:4) MOVLQZX AX,R8
0007 (sl.go:4) CMPQ    BP,R8
0008 (sl.go:4) JLS     ,10
0009 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
0010 (sl.go:4) CMPQ    BX,BP
0011 (sl.go:4) JHI     ,9
0012 (sl.go:4) MOVQL   BP,BP
0013 (sl.go:4) SUBL    BX,BP
0014 (sl.go:4) MOVL    BP,.noname+32(FP)
0015 (sl.go:4) MOVL    AX,BP
0016 (sl.go:4) SUBL    BX,BP
0017 (sl.go:4) MOVL    BP,.noname+36(FP)
0018 (sl.go:4) IMULQ   $4,BX
0019 (sl.go:4) ADDQ    s+0(FP),BX
0020 (sl.go:4) MOVQ    BX,.noname+24(FP)
0021 (sl.go:4) RET     ,


after:

0000 (sl.go:3) TEXT    h+0(SB),$16-40
0001 (sl.go:4) MOVQ    s+0(FP),BX
0002 (sl.go:4) MOVQ    BX,autotmp_0000+-16(SP)
0003 (sl.go:4) MOVL    s+8(FP),BX
0004 (sl.go:4) MOVL    s+12(FP),DX
0005 (sl.go:4) MOVL    hi+20(FP),CX
0006 (sl.go:4) MOVL    lo+16(FP),AX
0007 (sl.go:4) CMPL    DX,CX
0008 (sl.go:4) JCS     ,12
0009 (sl.go:4) CMPL    CX,AX
0010 (sl.go:4) JCS     ,12
0011 (sl.go:4) JMP     ,13
0012 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
0013 (sl.go:4) MOVL    CX,BX
0014 (sl.go:4) SUBL    AX,BX
0015 (sl.go:4) MOVL    BX,.noname+32(FP)
0016 (sl.go:4) MOVL    DX,BX
0017 (sl.go:4) SUBL    AX,BX
0018 (sl.go:4) MOVL    BX,.noname+36(FP)
0019 (sl.go:4) MOVLQZX AX,BX
0020 (sl.go:4) MOVQ    $4,BP
0021 (sl.go:4) IMULQ   BP,BX
0022 (sl.go:4) MOVQ    autotmp_0000+-16(SP),BP
0023 (sl.go:4) ADDQ    BP,BX
0024 (sl.go:4) MOVQ    BX,.noname+24(FP)
0025 (sl.go:4) RET     ,

[rog, 2012-04-16 14:30:42]

what does the code look like after the linker has done with it?

On 16 April 2012 13:56, Luuk van Dijk <lvd@google.com> wrote:
>
>
> On Mon, Apr 16, 2012 at 13:46, Anthony Martin <ality@pbrane.org> wrote:
>>
>> This is glorious. Over 1000 lines deleted.
>>
>> Do we have any slice heavy benchmarks to
>> see the effect it has?
>
>
> suggesions for realistic benchmarks are welcome.  the newly generated code
> tends to have some spurious loads and stores that the registerizer doesn't
> seem to catch, i'll look closer at that, but on the other hand, this now
> handles /all OSLICExxx whereas the old code would leave some calls to
> runtime.slicexxx in.
>
> to get an idea here is the old/new 6g -S output for
>
> func h(s []int, lo, hi int) []int {
>         return s[lo:hi]
> }
>
> before:
>
> 0000 (sl.go:3) TEXT    h+0(SB),$0-40
> 0001 (sl.go:3) MOVL    s+12(FP),AX
> 0002 (sl.go:4) MOVL    lo+16(FP),BX
> 0003 (sl.go:4) MOVLQSX BX,BX
> 0004 (sl.go:4) MOVL    hi+20(FP),BP
> 0005 (sl.go:4) MOVLQSX BP,BP
> 0006 (sl.go:4) MOVLQZX AX,R8
> 0007 (sl.go:4) CMPQ    BP,R8
> 0008 (sl.go:4) JLS     ,10
> 0009 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
> 0010 (sl.go:4) CMPQ    BX,BP
> 0011 (sl.go:4) JHI     ,9
> 0012 (sl.go:4) MOVQL   BP,BP
> 0013 (sl.go:4) SUBL    BX,BP
> 0014 (sl.go:4) MOVL    BP,.noname+32(FP)
> 0015 (sl.go:4) MOVL    AX,BP
> 0016 (sl.go:4) SUBL    BX,BP
> 0017 (sl.go:4) MOVL    BP,.noname+36(FP)
> 0018 (sl.go:4) IMULQ   $4,BX
> 0019 (sl.go:4) ADDQ    s+0(FP),BX
> 0020 (sl.go:4) MOVQ    BX,.noname+24(FP)
> 0021 (sl.go:4) RET     ,
>
>
> after:
>
> 0000 (sl.go:3) TEXT    h+0(SB),$16-40
> 0001 (sl.go:4) MOVQ    s+0(FP),BX
> 0002 (sl.go:4) MOVQ    BX,autotmp_0000+-16(SP)
> 0003 (sl.go:4) MOVL    s+8(FP),BX
> 0004 (sl.go:4) MOVL    s+12(FP),DX
> 0005 (sl.go:4) MOVL    hi+20(FP),CX
> 0006 (sl.go:4) MOVL    lo+16(FP),AX
> 0007 (sl.go:4) CMPL    DX,CX
> 0008 (sl.go:4) JCS     ,12
> 0009 (sl.go:4) CMPL    CX,AX
> 0010 (sl.go:4) JCS     ,12
> 0011 (sl.go:4) JMP     ,13
> 0012 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
> 0013 (sl.go:4) MOVL    CX,BX
> 0014 (sl.go:4) SUBL    AX,BX
> 0015 (sl.go:4) MOVL    BX,.noname+32(FP)
> 0016 (sl.go:4) MOVL    DX,BX
> 0017 (sl.go:4) SUBL    AX,BX
> 0018 (sl.go:4) MOVL    BX,.noname+36(FP)
> 0019 (sl.go:4) MOVLQZX AX,BX
> 0020 (sl.go:4) MOVQ    $4,BP
> 0021 (sl.go:4) IMULQ   BP,BX
> 0022 (sl.go:4) MOVQ    autotmp_0000+-16(SP),BP
> 0023 (sl.go:4) ADDQ    BP,BX
> 0024 (sl.go:4) MOVQ    BX,.noname+24(FP)
> 0025 (sl.go:4) RET     ,
>
>
>

[lvd, 2012-04-16 14:44:49]

On Mon, Apr 16, 2012 at 16:30, roger peppe <rogpeppe@gmail.com> wrote:

> what does the code look like after the linker has done with it?
>
>
pre:

      2 002000  main.h               | (3)      TEXT
 main.h+0(SB),$171798691840
      3 002000  65488b0c25a0080000   | (3)      MOVQ    2208(GS),CX
      4 002009  483b21               | (3)      CMPQ    SP,(CX)
      5 00200c  7705                 | (3)      JHI     ,2013
      6 00200e  e8b9fe0000           | (3)      CALL
 ,11ecc+runtime.morestack40
      7 002013  8b442414             | (3)      MOVL    main.s+20(SP),AX
      8 002017  8b5c2418             | (5)      MOVL    main.lo+24(SP),BX
      9 00201b  4863db               | (5)      MOVLQSX BX,BX
     10 00201e  8b6c241c             | (5)      MOVL    main.hi+28(SP),BP
     11 002022  4863ed               | (5)      MOVLQSX BP,BP
     12 002025  448bc0               | (5)      MOVLQZX AX,R8
     13 002028  4c39c5               | (5)      CMPQ    BP,R8
     14 00202b  7605                 | (5)      JLS     ,2032
     15 00202d  e8eba70000           | (5)      CALL
 ,c81d+runtime.panicslice
     16 002032  4839eb               | (5)      CMPQ    BX,BP
     17 002035  77f6                 | (5)      JHI     ,202d
     18 002037  89ed                 | (5)      MOVQL   BP,BP
     19 002039  29dd                 | (5)      SUBL    BX,BP
     20 00203b  896c2428             | (5)      MOVL
 BP,main..noname+40(SP)
     21 00203f  89c5                 | (5)      MOVL    AX,BP
     22 002041  29dd                 | (5)      SUBL    BX,BP
     23 002043  896c242c             | (5)      MOVL
 BP,main..noname+44(SP)
     24 002047  486bdb04             | (5)      IMULQ   $4,BX
     25 00204b  48035c2408           | (5)      ADDQ    main.s+8(SP),BX
     26 002050  48895c2420           | (5)      MOVQ
 BX,main..noname+32(SP)
     27 002055  c3                   | (5)      RET     ,




post:

      2 002000  main.h               | (3)      TEXT
 main.h+0(SB),$171798691856
      3 002000  65488b0c25a0080000   | (3)      MOVQ    2208(GS),CX
      4 002009  483b21               | (3)      CMPQ    SP,(CX)
      5 00200c  7705                 | (3)      JHI     ,2013
      6 00200e  e8e1fe0000           | (3)      CALL
 ,11ef4+runtime.morestack40
      7 002013  4883ec10             | (3)      SUBQ    $16,SP
      8 002017  488b5c2418           | (5)      MOVQ    main.s+24(SP),BX
      9 00201c  48891c24             | (5)      MOVQ
 BX,main.autotmp_0000+0(SP)
     10 002020  8b5c2420             | (5)      MOVL    main.s+32(SP),BX
     11 002024  8b542424             | (5)      MOVL    main.s+36(SP),DX
     12 002028  8b4c242c             | (5)      MOVL    main.hi+44(SP),CX
     13 00202c  8b442428             | (5)      MOVL    main.lo+40(SP),AX
     14 002030  39ca                 | (5)      CMPL    DX,CX
     15 002032  7232                 | (5)      JCS     ,2066
     16 002034  39c1                 | (5)      CMPL    CX,AX
     17 002036  722e                 | (5)      JCS     ,2066
     18 002038  89cb                 | (5)      MOVL    CX,BX
     19 00203a  29c3                 | (5)      SUBL    AX,BX
     20 00203c  895c2438             | (5)      MOVL
 BX,main..noname+56(SP)
     21 002040  89d3                 | (5)      MOVL    DX,BX
     22 002042  29c3                 | (5)      SUBL    AX,BX
     23 002044  895c243c             | (5)      MOVL
 BX,main..noname+60(SP)
     24 002048  8bd8                 | (5)      MOVLQZX AX,BX
     25 00204a  48c7c504000000       | (5)      MOVQ    $4,BP
     26 002051  480fafdd             | (5)      IMULQ   BP,BX
     27 002055  488b2c24             | (5)      MOVQ
 main.autotmp_0000+0(SP),BP
     28 002059  4801eb               | (5)      ADDQ    BP,BX
     29 00205c  48895c2430           | (5)      MOVQ
 BX,main..noname+48(SP)
     30 002061  4883c410             | (5)      ADDQ    $16,SP
     31 002065  c3                   | (5)      RET     ,
     32 002066  e8daa70000           | (5)      CALL
 ,c845+runtime.panicslice
     33 00206b  ebcb                 | (5)      JMP     ,2038




> On 16 April 2012 13:56, Luuk van Dijk <lvd@google.com> wrote:
> >
> >
> > On Mon, Apr 16, 2012 at 13:46, Anthony Martin <ality@pbrane.org> wrote:
> >>
> >> This is glorious. Over 1000 lines deleted.
> >>
> >> Do we have any slice heavy benchmarks to
> >> see the effect it has?
> >
> >
> > suggesions for realistic benchmarks are welcome.  the newly generated
> code
> > tends to have some spurious loads and stores that the registerizer
> doesn't
> > seem to catch, i'll look closer at that, but on the other hand, this now
> > handles /all OSLICExxx whereas the old code would leave some calls to
> > runtime.slicexxx in.
> >
> > to get an idea here is the old/new 6g -S output for
> >
> > func h(s []int, lo, hi int) []int {
> >         return s[lo:hi]
> > }
> >
> > before:
> >
> > 0000 (sl.go:3) TEXT    h+0(SB),$0-40
> > 0001 (sl.go:3) MOVL    s+12(FP),AX
> > 0002 (sl.go:4) MOVL    lo+16(FP),BX
> > 0003 (sl.go:4) MOVLQSX BX,BX
> > 0004 (sl.go:4) MOVL    hi+20(FP),BP
> > 0005 (sl.go:4) MOVLQSX BP,BP
> > 0006 (sl.go:4) MOVLQZX AX,R8
> > 0007 (sl.go:4) CMPQ    BP,R8
> > 0008 (sl.go:4) JLS     ,10
> > 0009 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
> > 0010 (sl.go:4) CMPQ    BX,BP
> > 0011 (sl.go:4) JHI     ,9
> > 0012 (sl.go:4) MOVQL   BP,BP
> > 0013 (sl.go:4) SUBL    BX,BP
> > 0014 (sl.go:4) MOVL    BP,.noname+32(FP)
> > 0015 (sl.go:4) MOVL    AX,BP
> > 0016 (sl.go:4) SUBL    BX,BP
> > 0017 (sl.go:4) MOVL    BP,.noname+36(FP)
> > 0018 (sl.go:4) IMULQ   $4,BX
> > 0019 (sl.go:4) ADDQ    s+0(FP),BX
> > 0020 (sl.go:4) MOVQ    BX,.noname+24(FP)
> > 0021 (sl.go:4) RET     ,
> >
> >
> > after:
> >
> > 0000 (sl.go:3) TEXT    h+0(SB),$16-40
> > 0001 (sl.go:4) MOVQ    s+0(FP),BX
> > 0002 (sl.go:4) MOVQ    BX,autotmp_0000+-16(SP)
> > 0003 (sl.go:4) MOVL    s+8(FP),BX
> > 0004 (sl.go:4) MOVL    s+12(FP),DX
> > 0005 (sl.go:4) MOVL    hi+20(FP),CX
> > 0006 (sl.go:4) MOVL    lo+16(FP),AX
> > 0007 (sl.go:4) CMPL    DX,CX
> > 0008 (sl.go:4) JCS     ,12
> > 0009 (sl.go:4) CMPL    CX,AX
> > 0010 (sl.go:4) JCS     ,12
> > 0011 (sl.go:4) JMP     ,13
> > 0012 (sl.go:4) CALL    ,runtime.panicslice+0(SB)
> > 0013 (sl.go:4) MOVL    CX,BX
> > 0014 (sl.go:4) SUBL    AX,BX
> > 0015 (sl.go:4) MOVL    BX,.noname+32(FP)
> > 0016 (sl.go:4) MOVL    DX,BX
> > 0017 (sl.go:4) SUBL    AX,BX
> > 0018 (sl.go:4) MOVL    BX,.noname+36(FP)
> > 0019 (sl.go:4) MOVLQZX AX,BX
> > 0020 (sl.go:4) MOVQ    $4,BP
> > 0021 (sl.go:4) IMULQ   BP,BX
> > 0022 (sl.go:4) MOVQ    autotmp_0000+-16(SP),BP
> > 0023 (sl.go:4) ADDQ    BP,BX
> > 0024 (sl.go:4) MOVQ    BX,.noname+24(FP)
> > 0025 (sl.go:4) RET     ,
> >
> >
> >
>

[minux1, 2012-04-17 18:18:05]

On 2012/04/16 11:33:02, lvd wrote:
> 6g and 8g pass all tests
> for 5g i need to be able to test somewhere
after removed the unused variable (Node m) in 5g/gsubr.c, ./all.bash failed
test/nilptr.go:
run        nilptr.go           : incorrect output
panic: memory reference did not panic
goroutine 1 [running]:
main._func_001(0x1ce0c, 0x10e60)
	/mnt/work2/tmp/gobuilder/linux-arm/test/nilptr.go:46 +0x78
main.shouldPanic(0x10e60, 0x0)
	/mnt/work2/tmp/gobuilder/linux-arm/test/nilptr.go:49 +0x50
main.main()
	/mnt/work2/tmp/gobuilder/linux-arm/test/nilptr.go:33 +0x94
goroutine 2 [syscall]:
created by runtime.main

Also, I think the test (in /test) is slower (14min vs. 7min) with this CL
applied.

[minux1, 2012-04-17 18:54:49]

This should fix the test failure (./all.bash passed on the Linux/ARM builder).

I need more investigations for the possible performance regression.

http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c
File src/cmd/5g/gsubr.c (right):

http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208
src/cmd/5g/gsubr.c:1208: //	TODO lvd
void
checkref(Node *n)
{
    Node m1, m2;

    if(n->type->type->width < unmappedzero)
        return;

    regalloc(&m1, types[TUINTPTR], n);
    regalloc(&m2, types[TUINT8], n);
    cgen(n, &m1);
    m1.xoffset = 0; 
    m1.op = OINDREG;
    m1.type = types[TUINT8];
    gins(AMOVBU, &m1, &m2);
    regfree(&m2);
    regfree(&m1);
}

[lvd, 2012-04-17 20:19:41]

thank you!  i hadn't gotten around to figuring out that missing bit and i
didn't have an arm to play on.

On Tue, Apr 17, 2012 at 20:54, <minux.ma@gmail.com> wrote:

> This should fix the test failure (./all.bash passed on the Linux/ARM
> builder).
>
> I need more investigations for the possible performance regression.
>
>
for 5g, the original cgen_inline wasn't even called afaict, so it would
always call the pkg/runtime functions.   a factor 2 sounds excessive though.


>
>
http://codereview.appspot.com/**5966075/diff/24001/src/cmd/5g/**gsubr.c<http:...
> File src/cmd/5g/gsubr.c (right):
>
> http://codereview.appspot.com/**5966075/diff/24001/src/cmd/5g/**
>
gsubr.c#newcode1208<http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208>
> src/cmd/5g/gsubr.c:1208: //     TODO lvd
> void
> checkref(Node *n)
> {
>    Node m1, m2;
>
>    if(n->type->type->width < unmappedzero)
>        return;
>
>    regalloc(&m1, types[TUINTPTR], n);
>    regalloc(&m2, types[TUINT8], n);
>    cgen(n, &m1);
>    m1.xoffset = 0;
>    m1.op = OINDREG;
>    m1.type = types[TUINT8];
>    gins(AMOVBU, &m1, &m2);
>    regfree(&m2);
>    regfree(&m1);
> }
>
>
http://codereview.appspot.com/**5966075/<http://codereview.appspot.com/5966075/>
>

[lvd, 2012-04-17 20:39:30]

On Tue, Apr 17, 2012 at 22:19, Luuk van Dijk <lvd@google.com> wrote:

> thank you!  i hadn't gotten around to figuring out that missing bit and i
> didn't have an arm to play on.
>
> On Tue, Apr 17, 2012 at 20:54, <minux.ma@gmail.com> wrote:
>
>> This should fix the test failure (./all.bash passed on the Linux/ARM
>> builder).
>>
>> I need more investigations for the possible performance regression.
>>
>>
> for 5g, the original cgen_inline wasn't even called afaict, so it would
> always call the pkg/runtime functions.   a factor 2 sounds excessive though.
>

esp since time run in test and go run in test/bench/go1 show no noticable
difference on darwin/amd64 and darwin/386...


>
>
>>
>>
http://codereview.appspot.com/**5966075/diff/24001/src/cmd/5g/**gsubr.c<http:...
>> File src/cmd/5g/gsubr.c (right):
>>
>> http://codereview.appspot.com/**5966075/diff/24001/src/cmd/5g/**
>>
gsubr.c#newcode1208<http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208>
>> src/cmd/5g/gsubr.c:1208: //     TODO lvd
>> void
>> checkref(Node *n)
>> {
>>    Node m1, m2;
>>
>>    if(n->type->type->width < unmappedzero)
>>        return;
>>
>>    regalloc(&m1, types[TUINTPTR], n);
>>    regalloc(&m2, types[TUINT8], n);
>>    cgen(n, &m1);
>>    m1.xoffset = 0;
>>    m1.op = OINDREG;
>>    m1.type = types[TUINT8];
>>    gins(AMOVBU, &m1, &m2);
>>    regfree(&m2);
>>    regfree(&m1);
>> }
>>
>>
http://codereview.appspot.com/**5966075/<http://codereview.appspot.com/5966075/>
>>
>
>

[lvd, 2012-04-17 20:41:11]

On Tue, Apr 17, 2012 at 22:39, Luuk van Dijk <lvd@google.com> wrote:

> http://codereview.appspot.com/**5966075/diff/24001/src/cmd/5g/**
>>>
gsubr.c#newcode1208<http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208>
>>> src/cmd/5g/gsubr.c:1208: //     TODO lvd
>>> void
>>> checkref(Node *n)
>>> {
>>>    Node m1, m2;
>>>
>>>    if(n->type->type->width < unmappedzero)
>>>        return;
>>>
>>>    regalloc(&m1, types[TUINTPTR], n);
>>>    regalloc(&m2, types[TUINT8], n);
>>>    cgen(n, &m1);
>>>    m1.xoffset = 0;
>>>    m1.op = OINDREG;
>>>    m1.type = types[TUINT8];
>>>    gins(AMOVBU, &m1, &m2);
>>>    regfree(&m2);
>>>    regfree(&m1);
>>> }
>>>
>>>
http://codereview.appspot.com/**5966075/<http://codereview.appspot.com/5966075/>
>>>
>>
>>
>
i added your checkref to my CL.  thanks again.

/L

[dave_cheney.net, 2012-04-18 00:56:22]

I hand patched minux's suggested checkref only my 5g system and now
all the tests pass, however I share the concern that there has been a
performance regression. I'll dig into it more today but anecdotally
src/test/run took 23 minutes (i'll confirm the pre cl times) and
anything that needs to use go/parser etc (so the go tool, the api
tool) take signficantly longer and appear to be cpu bound.

Cheers

Dave

On Wed, Apr 18, 2012 at 6:41 AM, Luuk van Dijk <lvd@google.com> wrote:
>
>
> On Tue, Apr 17, 2012 at 22:39, Luuk van Dijk <lvd@google.com> wrote:
>>>>
>>>>
>>>>
http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208
>>>> src/cmd/5g/gsubr.c:1208: //     TODO lvd
>>>> void
>>>> checkref(Node *n)
>>>> {
>>>>    Node m1, m2;
>>>>
>>>>    if(n->type->type->width < unmappedzero)
>>>>        return;
>>>>
>>>>    regalloc(&m1, types[TUINTPTR], n);
>>>>    regalloc(&m2, types[TUINT8], n);
>>>>    cgen(n, &m1);
>>>>    m1.xoffset = 0;
>>>>    m1.op = OINDREG;
>>>>    m1.type = types[TUINT8];
>>>>    gins(AMOVBU, &m1, &m2);
>>>>    regfree(&m2);
>>>>    regfree(&m1);
>>>> }
>>>>
>>>> http://codereview.appspot.com/5966075/
>>>
>>>
>>
>
> i added your checkref to my CL.  thanks again.
>
> /L

[lvd, 2012-04-18 05:46:09]

On Wed, Apr 18, 2012 at 02:56, Dave Cheney <dave@cheney.net> wrote:

> I hand patched minux's suggested checkref only my 5g system and now
> all the tests pass, however I share the concern that there has been a
> performance regression. I'll dig into it more today but anecdotally
> src/test/run took 23 minutes (i'll confirm the pre cl times) and
>

wow, that is worrying.  thanks for investigating.


> anything that needs to use go/parser etc (so the go tool, the api
> tool) take signficantly longer and appear to be cpu bound.
>
> Cheers
>
> Dave
>
> On Wed, Apr 18, 2012 at 6:41 AM, Luuk van Dijk <lvd@google.com> wrote:
> >
> >
> > On Tue, Apr 17, 2012 at 22:39, Luuk van Dijk <lvd@google.com> wrote:
> >>>>
> >>>>
> >>>>
>
http://codereview.appspot.com/5966075/diff/24001/src/cmd/5g/gsubr.c#newcode1208
> >>>> src/cmd/5g/gsubr.c:1208: //     TODO lvd
> >>>> void
> >>>> checkref(Node *n)
> >>>> {
> >>>>    Node m1, m2;
> >>>>
> >>>>    if(n->type->type->width < unmappedzero)
> >>>>        return;
> >>>>
> >>>>    regalloc(&m1, types[TUINTPTR], n);
> >>>>    regalloc(&m2, types[TUINT8], n);
> >>>>    cgen(n, &m1);
> >>>>    m1.xoffset = 0;
> >>>>    m1.op = OINDREG;
> >>>>    m1.type = types[TUINT8];
> >>>>    gins(AMOVBU, &m1, &m2);
> >>>>    regfree(&m2);
> >>>>    regfree(&m1);
> >>>> }
> >>>>
> >>>> http://codereview.appspot.com/5966075/
> >>>
> >>>
> >>
> >
> > i added your checkref to my CL.  thanks again.
> >
> > /L
>

[lvd, 2012-04-18 06:19:38]

actually i didn't look close enough, there is a regression in
test/benchmark/go1, for JSONDecode on darwin/amd64, i'll look closer there.

benchmark                old ns/op    new ns/op    delta
BenchmarkBinaryTree17   5181384000   5095200000   -1.66%
BenchmarkFannkuch11     5556846000   5237117000   -5.75%
BenchmarkGobDecode        29561800     21883260  -25.97%
BenchmarkGobEncode        12907780     12791170   -0.90%
BenchmarkGzip                87800        87848   +0.05%
BenchmarkGunzip               6350         6267   -1.31%
BenchmarkJSONEncode       74979900     76979250   +2.67%
BenchmarkJSONDecode      341845000    522175800  +52.75%
BenchmarkRevcomp25M     1131012000   1121440000   -0.85%
BenchmarkTemplate        654604800    716804800   +9.50%

benchmark                 old MB/s     new MB/s  speedup
BenchmarkGobDecode           25.96        35.07    1.35x
BenchmarkGobEncode           59.46        60.00    1.01x
BenchmarkJSONEncode          25.88        25.21    0.97x
BenchmarkJSONDecode           5.68         3.72    0.65x
BenchmarkRevcomp25M         224.73       226.64    1.01x
BenchmarkTemplate             2.96         2.71    0.92x

[lvd, 2012-04-18 06:29:48]

actually, on 386 it's terrible too, and even on amd64, there's a 33%
slowdown for sliceslice.  this needs some more work.

i used these benchmarks

package sl_test

import "testing"

var a [100]byte

const s = "alloallo"

func BenchmarkSliceSlice(b *testing.B) {
s1 := a[:]
lo, hi := 3, 97
var s2 []byte
        for i := 0; i < b.N; i++ {
s2 = s1[lo: hi]
        }
_ = s2
}

func BenchmarkSliceStr(b *testing.B) {
s1 := s[:]
lo, hi := 3, 7
var s2 string
        for i := 0; i < b.N; i++ {
s2 = s1[lo: hi]
        }
_ = s2
}


func BenchmarkSliceArr(b *testing.B) {
lo, hi := 3, 7
var s2 []byte
        for i := 0; i < b.N; i++ {
s2 = a[lo: hi]
        }
_ = s2
}

[lvd, 2012-04-18 06:42:27]

On Wed, Apr 18, 2012 at 08:29, Luuk van Dijk <lvd@google.com> wrote:

> actually, on 386 it's terrible too, and even on amd64, there's a 33%
> slowdown for sliceslice.  this needs some more work.


part of it seems to be due to the safeexpr(n->left) i turned into a
copyexpr(n->left) in the last revision to fix a crashing case.   hold off
your reviews, i'll hammer on it some more.

[lvd, 2012-04-18 08:01:18]

On Wed, Apr 18, 2012 at 08:42, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Wed, Apr 18, 2012 at 08:29, Luuk van Dijk <lvd@google.com> wrote:
>
>> actually, on 386 it's terrible too, and even on amd64, there's a 33%
>> slowdown for sliceslice.  this needs some more work.
>
>
> part of it seems to be due to the safeexpr(n->left) i turned into a
> copyexpr(n->left) in the last revision to fix a crashing case.   hold off
> your reviews, i'll hammer on it some more.
>
>
i uploaded a new revision that undoes that copyexpr except in one case for
which i still have to find a better fix (and the root cause).

this gets the 'realistic' benchmarks sane again on amd64 and 386 with some
big wins, presumably due to slicestr getting inlined now, and some small
losses due to sliceslice and slicearr taking 25% longer (1.85 vs. 1.4ns)

i expect the factor 2 on arm to be gone too.

looking at the generated assembly, i can only explain it from the new
layout of the bounds check which goes from

0016 (sl_test.go:14) MOVLQSX SI,BX
0017 (sl_test.go:14) MOVLQSX DX,BP
0018 (sl_test.go:14) MOVLQZX CX,R8
0019 (sl_test.go:14) CMPQ    BP,R8
0020 (sl_test.go:14) JLS     ,22
0021 (sl_test.go:14) CALL    ,runtime.panicslice+0(SB)
0022 (sl_test.go:14) CMPQ    BX,BP
0023 (sl_test.go:14) JHI     ,21

to

0017 (sl_test.go:14) MOVL    R10,DX
0018 (sl_test.go:14) MOVL    DI,CX
0019 (sl_test.go:14) MOVL    R8,AX
0020 (sl_test.go:14) CMPL    R10,DI
0021 (sl_test.go:14) JCS     ,25
0022 (sl_test.go:14) CMPL    DI,R8
0023 (sl_test.go:14) JCS     ,25
0024 (sl_test.go:14) JMP     ,26
0025 (sl_test.go:14) CALL    ,runtime.panicslice+0(SB)

I'll continue to work on this.

[minux1, 2012-04-19 17:45:34]

beside this small typo, I can confirm that the performance regression on ARM is
fixed.

go run test/run.go
old:
real 6m9.318s   user 10m42.94s   sys 1m0.71s
new:
real 5m55.78s   user 10m5.82s   sys 1m0.71s

http://codereview.appspot.com/5966075/diff/30022/src/cmd/5g/gsubr.c
File src/cmd/5g/gsubr.c (right):

http://codereview.appspot.com/5966075/diff/30022/src/cmd/5g/gsubr.c#newcode1199
src/cmd/5g/gsubr.c:1199: bsdvoid
sorry, why is bsdvoid here?

[lvd, 2012-04-19 18:13:46]

On 2012/04/19 17:45:34, minux wrote:
> beside this small typo, I can confirm that the performance regression on ARM
is
> fixed.
> 

thank you!

> go run test/run.go
> old:
> real 6m9.318s   user 10m42.94s   sys 1m0.71s
> new:
> real 5m55.78s   user 10m5.82s   sys 1m0.71s
> 
> http://codereview.appspot.com/5966075/diff/30022/src/cmd/5g/gsubr.c
> File src/cmd/5g/gsubr.c (right):
> 
>
http://codereview.appspot.com/5966075/diff/30022/src/cmd/5g/gsubr.c#newcode1199
> src/cmd/5g/gsubr.c:1199: bsdvoid
> sorry, why is bsdvoid here?

i have absolutely no idea :-)  i was wondering what  dave was talkiing about. 
it must have been me telling emacs the indent mode and missing a ctrl or a shift
or so.

[dave_cheney.net, 2012-04-19 21:59:34]

LGTM.

Before, core i5 darwin/amd64 

odessa(~/go/test) % time ./run
0 known bugs; 0 unexpected bugs

real	0m46.944s
user	0m32.189s
sys	0m11.451s

After

odessa(~/go/test) % time ./run
0 known bugs; 0 unexpected bugs

real	0m44.288s
user	0m30.566s
sys	0m10.983s

[rsc, 2012-06-02 22:47:58]

LGTM

I'm sorry it took me so long to get to this review. I think you're still away,
so I'll patch this in, address the comments, and submit.

http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2347
src/cmd/gc/walk.c:2347: nochk = n->etype;
This is now n->bounded.

http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2359
src/cmd/gc/walk.c:2359: if (isconst(bound, CTINT))
All these isconst need to check the range of the value too. There can be very
large integer constants that won't fit in the int64s here.

http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2403
src/cmd/gc/walk.c:2403: if(n->op == OSLICE) {
I think OSLICESTR should be here too.

[dave_cheney.net, 2012-06-02 22:49:17]

Woot. 

On 03/06/2012, at 8:47, rsc@golang.org wrote:

> LGTM
> 
> I'm sorry it took me so long to get to this review. I think you're still
> away, so I'll patch this in, address the comments, and submit.
> 
> 
> http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c
> File src/cmd/gc/walk.c (right):
> 
>
http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2347
> src/cmd/gc/walk.c:2347: nochk = n->etype;
> This is now n->bounded.
> 
>
http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2359
> src/cmd/gc/walk.c:2359: if (isconst(bound, CTINT))
> All these isconst need to check the range of the value too. There can be
> very large integer constants that won't fit in the int64s here.
> 
>
http://codereview.appspot.com/5966075/diff/30022/src/cmd/gc/walk.c#newcode2403
> src/cmd/gc/walk.c:2403: if(n->op == OSLICE) {
> I think OSLICESTR should be here too.
> 
> http://codereview.appspot.com/5966075/

[rsc, 2012-06-03 02:51:03]

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

cmd/gc: inline slice[arr,str] in the frontend (mostly).

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

Committer: Russ Cox <rsc@golang.org>

[dave_cheney.net, 2012-06-03 03:54:25]

Sorry, this doesn't compile under 5c

cmd/5g
/home/dfc/go/src/cmd/5g/gsubr.c:1146:1: error: unknown type name 'bsdvoid'
bsdvoid
^
/home/dfc/go/src/cmd/5g/gsubr.c:1152:3: error: non-void function
'checkref' should return a value [-Wreturn-type]
                return;
                ^
2 errors generated.
go tool dist: FAILED: clang -Wall -Wno-sign-compare
-Wno-missing-braces -Wno-parentheses -Wno-unknown-pragmas -Wno-switch
-Wno-comment -Werror -fno-common -ggdb -O2 -Wnoc
pando(~/go/src) %


On Sun, Jun 3, 2012 at 12:51 PM,  <rsc@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=9acc060bf892 ***
>
> cmd/gc: inline slice[arr,str] in the frontend (mostly).
>
> R=rsc, ality, rogpeppe, minux.ma, dave
> CC=golang-dev
> http://codereview.appspot.com/5966075
>
> Committer: Russ Cox <rsc@golang.org>
>
>
> http://codereview.appspot.com/5966075/

[rsc, 2012-06-03 03:55:43]

On Sat, Jun 2, 2012 at 11:54 PM, Dave Cheney <dave@cheney.net> wrote:
> Sorry, this doesn't compile under 5c

Sorry, I meant to fix that - minux pointed it out.
If you s/bsd// is all okay?

Russ

[dave_cheney.net, 2012-06-03 03:56:44]

Yup.

On Sun, Jun 3, 2012 at 1:55 PM, Russ Cox <rsc@golang.org> wrote:
> On Sat, Jun 2, 2012 at 11:54 PM, Dave Cheney <dave@cheney.net> wrote:
>> Sorry, this doesn't compile under 5c
>
> Sorry, I meant to fix that - minux pointed it out.
> If you s/bsd// is all okay?
>
> Russ

[rsc, 2012-06-03 03:58:17]

On Sat, Jun 2, 2012 at 11:56 PM, Dave Cheney <dave@cheney.net> wrote:
> Yup.

submitted as 6275048.

[lvd, 2012-06-13 07:50:07]

what a nice surprise to find coming back from holiday. thanks!

On Sun, Jun 3, 2012 at 5:57 AM, Russ Cox <rsc@golang.org> wrote:

> On Sat, Jun 2, 2012 at 11:56 PM, Dave Cheney <dave@cheney.net> wrote:
> > Yup.
>
> submitted as 6275048.
>

[lvd, 2012-10-01 13:46:29]

*** Abandoned ***