code review 92760044: runtime: use duff zero and copy to initialize memory

runtime: use duff zero and copy to initialize memory

benchmark                 old ns/op     new ns/op     delta
BenchmarkCopyFat512       1307          329           -74.83%
BenchmarkCopyFat256       666           169           -74.62%
BenchmarkCopyFat1024      2617          671           -74.36%
BenchmarkCopyFat128       343           89.0          -74.05%
BenchmarkCopyFat64        182           48.9          -73.13%
BenchmarkCopyFat32        103           28.8          -72.04%
BenchmarkClearFat128      102           46.6          -54.31%
BenchmarkClearFat512      344           167           -51.45%
BenchmarkClearFat64       50.5          26.5          -47.52%
BenchmarkClearFat256      147           87.2          -40.68%
BenchmarkClearFat32       22.7          16.4          -27.75%
BenchmarkClearFat1024     511           662           +29.55%

Fixes issue 7624

[khr, 2014-04-25 06:08:56]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[khr1, 2014-04-25 06:12:16]

Dave and Russ, please take a look.  I am in particular interested in
whether I have all the incantations to make sure peep.c, prog.c, and reg.c
make sense.  They pass all.bash but I'm not sure they will be correct in
all cases.



On Thu, Apr 24, 2014 at 11:08 PM, <khr@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://khr%40golang.org@code.google.com/p/go/
>
>
> Description:
> runtime: use duff zero and copy to initialize memory
>
> Please review this at https://codereview.appspot.com/92760044/
>
> Affected files (+584, -37 lines):
>   M src/cmd/5g/cgen.c
>   M src/cmd/5g/ggen.c
>   M src/cmd/5g/peep.c
>   M src/cmd/5g/prog.c
>   M src/cmd/5g/reg.c
>   M src/cmd/5l/5.out.h
>   M src/liblink/asm5.c
>   M src/liblink/obj5.c
>   M src/pkg/runtime/asm_arm.s
>   M src/pkg/runtime/memmove_test.go
>
>
>

[khr, 2014-04-25 06:13:36]

On 2014/04/25 06:12:16, khr1 wrote:
> Dave and Russ, please take a look.  I am in particular interested in
> whether I have all the incantations to make sure peep.c, prog.c, and reg.c
> make sense.  They pass all.bash but I'm not sure they will be correct in
> all cases.
> 
> 
> 
> On Thu, Apr 24, 2014 at 11:08 PM, <mailto:khr@golang.org> wrote:
> 
> > Reviewers: golang-codereviews,
> >
> > Message:
> > Hello mailto:golang-codereviews@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://khr%40golang.org@code.google.com/p/go/
> >
> >
> > Description:
> > runtime: use duff zero and copy to initialize memory
> >
> > Please review this at https://codereview.appspot.com/92760044/
> >
> > Affected files (+584, -37 lines):
> >   M src/cmd/5g/cgen.c
> >   M src/cmd/5g/ggen.c
> >   M src/cmd/5g/peep.c
> >   M src/cmd/5g/prog.c
> >   M src/cmd/5g/reg.c
> >   M src/cmd/5l/5.out.h
> >   M src/liblink/asm5.c
> >   M src/liblink/obj5.c
> >   M src/pkg/runtime/asm_arm.s
> >   M src/pkg/runtime/memmove_test.go
> >
> >
> >

PTAL

[bradfitz, 2014-04-25 13:26:13]

Fixes Issue 7624 in CL description?

[josharian, 2014-04-25 16:34:55]

Thanks for showing ARM some love! A few nits below.

Also, BenchmarkClearFat1024 looks anomalous. Do you have a sense for why it got
slower?

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/cgen.c
File src/cmd/5g/cgen.c (right):

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/cgen.c#newcode1508
src/cmd/5g/cgen.c:1508: regalloc(&src, types[tptr], &r1);
Looks like regalloc src/dst could be pulled out before the if statement. (Makes
it easier to see that the regfrees are correct.)

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/prog.c
File src/cmd/5g/prog.c (right):

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/prog.c#newcode96
src/cmd/5g/prog.c:96: // In addtion, duffzero reads R0,R1 and writes R1.  This
fact is
s/addtion/addition/ here and below

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/reg.c
File src/cmd/5g/reg.c (right):

https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/reg.c#newcode262
src/cmd/5g/reg.c:262: print("%P %x %x %x\n", p, r->use1.b[0], r->use2.b[0],
r->set.b[0]);
Add some context to the beginning of the format string?

https://codereview.appspot.com/92760044/diff/20001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/92760044/diff/20001/src/pkg/runtime/asm_arm.s#...
src/pkg/runtime/asm_arm.s:1157: MOVW	$0x99999999, R0  // testing
Leftover test code?

[khr, 2014-04-26 00:18:21]

On 2014/04/25 16:34:55, josharian wrote:
> Thanks for showing ARM some love! A few nits below.
> 
> Also, BenchmarkClearFat1024 looks anomalous. Do you have a sense for why it
got
> slower?
> 

It makes no sense to me.  The assembly is identical modulo register renaming and
it is consistently ~30% slower.  Code alignment maybe?  icache vs. dcache
conflict?  Grasping at straws...

old inner loop:
   728f0:       e4801004        str     r1, [r0], #4                            
                             
   728f4:       e1520000        cmp     r2, r0                                  
                             
   728f8:       1afffffc        bne     728f0
<runtime_test.BenchmarkClearFat1024+0x48>                       
 
new inner loop:
   71f58:       e4810004        str     r0, [r1], #4                            
                             
   71f5c:       e1520001        cmp     r2, r1                                  
                             
   71f60:       1afffffc        bne     71f58
<runtime_test.BenchmarkClearFat1024+0x48>                       



> https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/cgen.c
> File src/cmd/5g/cgen.c (right):
> 
>
https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/cgen.c#newcode1508
> src/cmd/5g/cgen.c:1508: regalloc(&src, types[tptr], &r1);
> Looks like regalloc src/dst could be pulled out before the if statement.
(Makes
> it easier to see that the regfrees are correct.)

Fixed.

> https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/prog.c
> File src/cmd/5g/prog.c (right):
> 
> https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/prog.c#newcode96
> src/cmd/5g/prog.c:96: // In addtion, duffzero reads R0,R1 and writes R1.  This
> fact is
> s/addtion/addition/ here and below
> 

Fixed.

> https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/reg.c
> File src/cmd/5g/reg.c (right):
> 
> https://codereview.appspot.com/92760044/diff/20001/src/cmd/5g/reg.c#newcode262
> src/cmd/5g/reg.c:262: print("%P %x %x %x\n", p, r->use1.b[0], r->use2.b[0],
> r->set.b[0]);
> Add some context to the beginning of the format string?

That's just my debuging code, I'll remove it.

> https://codereview.appspot.com/92760044/diff/20001/src/pkg/runtime/asm_arm.s
> File src/pkg/runtime/asm_arm.s (right):
> 
>
https://codereview.appspot.com/92760044/diff/20001/src/pkg/runtime/asm_arm.s#...
> src/pkg/runtime/asm_arm.s:1157: MOVW	$0x99999999, R0  // testing
> Leftover test code?

Yep, it is now gone.

[dave_cheney.net, 2014-04-26 14:55:41]

Some results from a pandaboard (dual Cortex-A9), I can't replicate the slowdown
on ClearFat1024

benchmark                 old ns/op     new ns/op     delta       
BenchmarkClearFat32       30.4          21.8          -28.29%     
BenchmarkClearFat64       67.2          35.1          -47.77%     
BenchmarkClearFat128      135           62.3          -53.85%     
BenchmarkClearFat256      195           115           -41.03%     
BenchmarkClearFat512      456           224           -50.88%     
BenchmarkClearFat1024     682           681           -0.15%      
BenchmarkCopyFat32        36.6          32.7          -10.66%     
BenchmarkCopyFat64        64.3          59.7          -7.15%      
BenchmarkCopyFat128       136           113           -16.91%     
BenchmarkCopyFat256       242           220           -9.09%      
BenchmarkCopyFat512       461           434           -5.86%      
BenchmarkCopyFat1024      893           893           +0.00%

[josharian, 2014-05-01 18:45:19]

> > Also, BenchmarkClearFat1024 looks anomalous. Do you have a sense for why it
> got
> > slower?
> 
> It makes no sense to me.  The assembly is identical modulo register renaming
and
> it is consistently ~30% slower.  Code alignment maybe?  icache vs. dcache
> conflict?  Grasping at straws...

I just hit something similar in another context. I believe that this might be an
artifact of interactions with other benchmarks, whose assembly did change. See
issue 7920.

[rsc, 2014-05-06 14:08:57]

LGTM

I've finally worked out the numbers there and I agree with what you had.

https://codereview.appspot.com/92760044/diff/40001/src/cmd/5g/peep.c
File src/cmd/5g/peep.c (right):

https://codereview.appspot.com/92760044/diff/40001/src/cmd/5g/peep.c#newcode1161
src/cmd/5g/peep.c:1161: if(v->type == D_REG) {
please add comment here

// R0 is zero, used by DUFFZERO, cannot be substituted.
// R1 is ptr to memory, used and set, cannot be substituted.

https://codereview.appspot.com/92760044/diff/40001/src/cmd/5g/peep.c#newcode1169
src/cmd/5g/peep.c:1169: if(v->type == D_REG) {
please add comment

// R0 is scratch, set by DUFFCOPY, cannot be substituted.
// R1, R2 are ptr to src, dst, used and set, cannot be substituted.

[khr, 2014-05-07 20:17:13]

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

runtime: use duff zero and copy to initialize memory

benchmark                 old ns/op     new ns/op     delta
BenchmarkCopyFat512       1307          329           -74.83%
BenchmarkCopyFat256       666           169           -74.62%
BenchmarkCopyFat1024      2617          671           -74.36%
BenchmarkCopyFat128       343           89.0          -74.05%
BenchmarkCopyFat64        182           48.9          -73.13%
BenchmarkCopyFat32        103           28.8          -72.04%
BenchmarkClearFat128      102           46.6          -54.31%
BenchmarkClearFat512      344           167           -51.45%
BenchmarkClearFat64       50.5          26.5          -47.52%
BenchmarkClearFat256      147           87.2          -40.68%
BenchmarkClearFat32       22.7          16.4          -27.75%
BenchmarkClearFat1024     511           662           +29.55%

Fixes issue 7624

LGTM=rsc
R=golang-codereviews, khr, bradfitz, josharian, dave, rsc
CC=golang-codereviews
https://codereview.appspot.com/92760044

[ianbramwel573, 2024-05-06 21:18:12]

On 2014/05/07 20:17:13, khr wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=e473e77e84ff ***
> 
> runtime: use duff zero and copy to initialize memory
> 
> benchmark                 old ns/op     new ns/op     delta
> BenchmarkCopyFat512       1307          329           -74.83%
> BenchmarkCopyFat256       666           169           -74.62%
> BenchmarkCopyFat1024      2617          671           -74.36%
> BenchmarkCopyFat128       343           89.0          -74.05%
> BenchmarkCopyFat64        182           48.9          -73.13%
> BenchmarkCopyFat32        103           28.8          -72.04%
> BenchmarkClearFat128      102           46.6          -54.31%
> BenchmarkClearFat512      344           167           -51.45%
> BenchmarkClearFat64       50.5          26.5          -47.52%
> BenchmarkClearFat256      147           87.2          -40.68%
> BenchmarkClearFat32       22.7          16.4          -27.75%
> BenchmarkClearFat1024     511           662           +29.55%
> 
> Fixes issue 7624
> 
> LGTM=rsc
> R=golang-codereviews, khr, bradfitz, josharian, dave, rsc
> CC=golang-codereviews
> https://codereview.appspot.com/92760044

>debugging

[ianbramwel573, 2024-05-06 21:18:14]

On 2014/05/07 20:17:13, khr wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=e473e77e84ff ***
> 
> runtime: use duff zero and copy to initialize memory
> 
> benchmark                 old ns/op     new ns/op     delta
> BenchmarkCopyFat512       1307          329           -74.83%
> BenchmarkCopyFat256       666           169           -74.62%
> BenchmarkCopyFat1024      2617          671           -74.36%
> BenchmarkCopyFat128       343           89.0          -74.05%
> BenchmarkCopyFat64        182           48.9          -73.13%
> BenchmarkCopyFat32        103           28.8          -72.04%
> BenchmarkClearFat128      102           46.6          -54.31%
> BenchmarkClearFat512      344           167           -51.45%
> BenchmarkClearFat64       50.5          26.5          -47.52%
> BenchmarkClearFat256      147           87.2          -40.68%
> BenchmarkClearFat32       22.7          16.4          -27.75%
> BenchmarkClearFat1024     511           662           +29.55%
> 
> Fixes issue 7624
> 
> LGTM=rsc
> R=golang-codereviews, khr, bradfitz, josharian, dave, rsc
> CC=golang-codereviews
> https://codereview.appspot.com/92760044

>debugging

[ianbramwel573, 2024-05-06 21:19:48]

On 2024/05/06 21:18:14, ianbramwel573 wrote:
> On 2014/05/07 20:17:13, khr wrote:
> > *** Submitted as https://code.google.com/p/go/source/detail?r=e473e77e84ff
***
> > 
> > runtime: use duff zero and copy to initialize memory
> > 
> > benchmark                 old ns/op     new ns/op     delta
> > BenchmarkCopyFat512       1307          329           -74.83%
> > BenchmarkCopyFat256       666           169           -74.62%
> > BenchmarkCopyFat1024      2617          671           -74.36%
> > BenchmarkCopyFat128       343           89.0          -74.05%
> > BenchmarkCopyFat64        182           48.9          -73.13%
> > BenchmarkCopyFat32        103           28.8          -72.04%
> > BenchmarkClearFat128      102           46.6          -54.31%
> > BenchmarkClearFat512      344           167           -51.45%
> > BenchmarkClearFat64       50.5          26.5          -47.52%
> > BenchmarkClearFat256      147           87.2          -40.68%
> > BenchmarkClearFat32       22.7          16.4          -27.75%
> > BenchmarkClearFat1024     511           662           +29.55%
> > 
> > Fixes issue 7624
> > 
> > LGTM=rsc
> > R=golang-codereviews, khr, bradfitz, josharian, dave, rsc
> > CC=golang-codereviews
> > https://codereview.appspot.com/92760044
> 
> >debugging issues
> JavaScript solve the problem.Bramwelian