code review 51740044: cmd/6g: faster memmove/memset-like code using unaligned...

cmd/6g: faster memmove/memset-like code using unaligned load/stores.

This changes makes sgen and clearfat use unaligned instructions for
the trailing bytes, like the runtime memmove does, resulting in faster
code when manipulating types whose size is not a multiple of 8.

[remyoudompheng, 2014-02-03 06:23:56]

Hello khr@golang.org, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

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

[remyoudompheng, 2014-02-03 06:27:57]

I have some trouble understanding why sgen needs to handle overlapping regions.

I have done the following benchmarks:

type A23 [23]byte
func BenchmarkAssign23(b *testing.B) {
        var a A23
        for i := 0; i < b.N; i++ {
                var x, y, z A23
                a, x, y = x, y, z
        }
        b.Log(a)
}

benchmark            old ns/op    new ns/op    delta
BenchmarkAssign23          120           31  -74.17%
BenchmarkAssign21          103           31  -69.90%
BenchmarkAssign19           59           36  -39.63%
BenchmarkAssign18           48           33  -31.33%
BenchmarkAssign17           38           38   +0.26%
BenchmarkAssign15          107           21  -79.81%
BenchmarkAssign13           90           19  -78.50%
BenchmarkAssign11           47           25  -45.47%
BenchmarkAssign9            27           26   -2.20%
BenchmarkAssign7            99           15  -83.96%

[khr, 2014-02-04 22:23:53]

On 2014/02/03 06:27:57, remyoudompheng wrote:
> I have some trouble understanding why sgen needs to handle overlapping
regions.
> 
> I have done the following benchmarks:
> 
> type A23 [23]byte
> func BenchmarkAssign23(b *testing.B) {
>         var a A23
>         for i := 0; i < b.N; i++ {
>                 var x, y, z A23
>                 a, x, y = x, y, z
>         }
>         b.Log(a)
> }
> 
> benchmark            old ns/op    new ns/op    delta
> BenchmarkAssign23          120           31  -74.17%
> BenchmarkAssign21          103           31  -69.90%
> BenchmarkAssign19           59           36  -39.63%
> BenchmarkAssign18           48           33  -31.33%
> BenchmarkAssign17           38           38   +0.26%
> BenchmarkAssign15          107           21  -79.81%
> BenchmarkAssign13           90           19  -78.50%
> BenchmarkAssign11           47           25  -45.47%
> BenchmarkAssign9            27           26   -2.20%
> BenchmarkAssign7            99           15  -83.96%

I don't really understand how overlapping ranges could happen either.  Put an
assert in there and see if all.bash triggers it.  If not, we should probably
reorg the code to do a very simple, slow algorithm in case of overlap and the
fast code otherwise.

Or maybe there's an argument to be made that overlap is impossible, and we just
leave the assert in?  I could be convinced...

[khr, 2014-02-05 06:03:44]

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/cgen.c#newcode1457
src/cmd/6g/cgen.c:1457: if(c > 4) {
Don't you need the overlap condition here as well?

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c
File src/cmd/6g/ggen.c (right):

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1053
src/cmd/6g/ggen.c:1053: nodconst(&z, types[TINT64], 0);
Shouldn't we use AX here?  It already has 0 in it.

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1060
src/cmd/6g/ggen.c:1060: nodconst(&z, types[TINT32], 0);
Same here.

[remyoudompheng, 2014-02-05 07:48:13]

On 2014/02/04 22:23:53, khr wrote:
> On 2014/02/03 06:27:57, remyoudompheng wrote:
> > I have some trouble understanding why sgen needs to handle overlapping
> regions.
> > 
> > I have done the following benchmarks:
> > 
> > type A23 [23]byte
> > func BenchmarkAssign23(b *testing.B) {
> >         var a A23
> >         for i := 0; i < b.N; i++ {
> >                 var x, y, z A23
> >                 a, x, y = x, y, z
> >         }
> >         b.Log(a)
> > }
> > 
> > benchmark            old ns/op    new ns/op    delta
> > BenchmarkAssign23          120           31  -74.17%
> > BenchmarkAssign21          103           31  -69.90%
> > BenchmarkAssign19           59           36  -39.63%
> > BenchmarkAssign18           48           33  -31.33%
> > BenchmarkAssign17           38           38   +0.26%
> > BenchmarkAssign15          107           21  -79.81%
> > BenchmarkAssign13           90           19  -78.50%
> > BenchmarkAssign11           47           25  -45.47%
> > BenchmarkAssign9            27           26   -2.20%
> > BenchmarkAssign7            99           15  -83.96%
> 
> I don't really understand how overlapping ranges could happen either.  Put an
> assert in there and see if all.bash triggers it.  If not, we should probably
> reorg the code to do a very simple, slow algorithm in case of overlap and the
> fast code otherwise.
> 
> Or maybe there's an argument to be made that overlap is impossible, and we
just
> leave the assert in?  I could be convinced...

It seems to be typical in case of chained function calls when the return value
becomes an argument and is shifted on the stack.

[remyoudompheng, 2014-02-07 00:53:31]

Hello khr@golang.org, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[khr, 2014-02-07 01:09:47]

On 2014/02/07 00:53:31, remyoudompheng wrote:
> Hello mailto:khr@golang.org, mailto:iant@golang.org, mailto:rsc@golang.org
(cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

cgen looks ok.  ggen still needs to use AX instead of $0 as the zero source.

[remyoudompheng, 2014-02-07 07:36:01]

Hello khr@golang.org, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[remyoudompheng, 2014-02-07 07:36:57]

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c
File src/cmd/6g/ggen.c (right):

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1053
src/cmd/6g/ggen.c:1053: nodconst(&z, types[TINT64], 0);
On 2014/02/05 06:03:44, khr wrote:
> Shouldn't we use AX here?  It already has 0 in it.

Done.

https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1060
src/cmd/6g/ggen.c:1060: nodconst(&z, types[TINT32], 0);
On 2014/02/05 06:03:44, khr wrote:
> Same here.

Done.

[khr, 2014-02-07 08:44:36]

On 2014/02/07 07:36:57, remyoudompheng wrote:
> https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c
> File src/cmd/6g/ggen.c (right):
> 
>
https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1053
> src/cmd/6g/ggen.c:1053: nodconst(&z, types[TINT64], 0);
> On 2014/02/05 06:03:44, khr wrote:
> > Shouldn't we use AX here?  It already has 0 in it.
> 
> Done.
> 
>
https://codereview.appspot.com/51740044/diff/20002/src/cmd/6g/ggen.c#newcode1060
> src/cmd/6g/ggen.c:1060: nodconst(&z, types[TINT32], 0);
> On 2014/02/05 06:03:44, khr wrote:
> > Same here.
> 
> Done.

LGTM.

[remyoudompheng, 2014-02-07 23:09:09]

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

cmd/6g: faster memmove/memset-like code using unaligned load/stores.

This changes makes sgen and clearfat use unaligned instructions for
the trailing bytes, like the runtime memmove does, resulting in faster
code when manipulating types whose size is not a multiple of 8.

LGTM=khr
R=khr, iant, rsc
CC=golang-codereviews
https://codereview.appspot.com/51740044

[khr, 2014-02-11 22:34:57]

On 2014/02/07 23:09:09, remyoudompheng wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=ec5195954667 ***
> 
> cmd/6g: faster memmove/memset-like code using unaligned load/stores.
> 
> This changes makes sgen and clearfat use unaligned instructions for
> the trailing bytes, like the runtime memmove does, resulting in faster
> code when manipulating types whose size is not a multiple of 8.
> 
> LGTM=khr
> R=khr, iant, rsc
> CC=golang-codereviews
> https://codereview.appspot.com/51740044

R=close