code review 9101048: cmd/gc: inline copy in frontend to call memmove directly.

cmd/gc: inline copy in frontend to call memmove directly.

A new node type OSPTR is added to refer to the data pointer of
strings and slices in a simple way during walk(). It will be
useful for future work on simplification of slice arithmetic.

benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                 9            8  -13.98%
BenchmarkCopy2Byte                14            8  -40.49%
BenchmarkCopy4Byte                13            8  -35.04%
BenchmarkCopy8Byte                13            8  -37.10%
BenchmarkCopy12Byte               14           12  -15.38%
BenchmarkCopy16Byte               14           12  -17.24%
BenchmarkCopy32Byte               19           14  -27.32%
BenchmarkCopy128Byte              31           26  -15.29%
BenchmarkCopy1024Byte            100           92   -7.50%
BenchmarkCopy1String              10            7  -28.99%
BenchmarkCopy2String              10            7  -28.06%
BenchmarkCopy4String              10            8  -22.69%
BenchmarkCopy8String              10            8  -23.30%
BenchmarkCopy12String             11           11   -5.88%
BenchmarkCopy16String             11           11   -5.08%
BenchmarkCopy32String             15           14   -6.58%
BenchmarkCopy128String            28           25  -10.60%
BenchmarkCopy1024String           95           95   +0.53%

[remyoudompheng, 2013-05-14 23:30:53]

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

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

[bradfitz, 2013-05-14 23:41:55]

Why does the memmove take uintptr instead of, say, *byte?



On Tue, May 14, 2013 at 4:30 PM, <remyoudompheng@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/gc: inline copy in frontend to call memmove directly.
>
> A new node type OSPTR is added to refer to the data pointer of
> strings and slices in a simple way during walk(). It will be
> useful for future work on simplification of slice arithmetic.
>
> benchmark                  old ns/op    new ns/op    delta
> BenchmarkCopy1Byte                10            4  -52.52%
> BenchmarkCopy2Byte                36           26  -28.02%
> BenchmarkCopy4Byte                74           55  -25.84%
> BenchmarkCopy8Byte                36           26  -27.75%
> BenchmarkCopy12Byte               74           55  -25.84%
> BenchmarkCopy16Byte               36           26  -28.02%
> BenchmarkCopy32Byte               36           26  -28.02%
> BenchmarkCopy128Byte              38           28  -24.61%
> BenchmarkCopy1024Byte            107           96  -10.28%
> BenchmarkCopy1String              27            4  -84.11%
> BenchmarkCopy2String              27           25   -4.44%
> BenchmarkCopy4String              56           54   -3.02%
> BenchmarkCopy8String              27           25   -4.81%
> BenchmarkCopy12String             56           54   -3.02%
> BenchmarkCopy16String             27           25   -4.44%
> BenchmarkCopy32String             27           25   -5.17%
> BenchmarkCopy128String            29           28   -4.05%
> BenchmarkCopy1024String           96           96   -0.41%
>
> benchmark                   old MB/s     new MB/s  speedup
> BenchmarkCopy1Byte             96.75       204.34    2.11x
> BenchmarkCopy2Byte             54.91        76.35    1.39x
> BenchmarkCopy4Byte             53.58        72.26    1.35x
> BenchmarkCopy8Byte            219.74       304.05    1.38x
> BenchmarkCopy12Byte           160.72       216.76    1.35x
> BenchmarkCopy16Byte           439.27       611.25    1.39x
> BenchmarkCopy32Byte           879.09      1222.41    1.39x
> BenchmarkCopy128Byte         3354.53      4437.76    1.32x
> BenchmarkCopy1024Byte        9490.74     10670.42    1.12x
> BenchmarkCopy1String           36.99       233.05    6.30x
> BenchmarkCopy2String           73.98        77.52    1.05x
> BenchmarkCopy4String           71.16        73.40    1.03x
> BenchmarkCopy8String          295.92       310.71    1.05x
> BenchmarkCopy12String         213.49       220.17    1.03x
> BenchmarkCopy16String         591.64       620.20    1.05x
> BenchmarkCopy32String        1182.17      1242.93    1.05x
> BenchmarkCopy128String       4320.98      4507.24    1.04x
> BenchmarkCopy1024String     10591.94     10633.94    1.00x
>
> Please review this at
https://codereview.appspot.**com/9101048/<https://codereview.appspot.com/9101...
>
> Affected files:
>   M src/cmd/5g/cgen.c
>   M src/cmd/5g/gsubr.c
>   M src/cmd/6g/cgen.c
>   M src/cmd/6g/gsubr.c
>   M src/cmd/8g/cgen.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/racewalk.c
>   M src/cmd/gc/runtime.go
>   M src/cmd/gc/typecheck.c
>   M src/cmd/gc/walk.c
>   M src/pkg/runtime/append_test.go
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[cshapiro1, 2013-05-14 23:47:49]

On 2013/05/14 23:41:55, bradfitz wrote:
> Why does the memmove take uintptr instead of, say, *byte?

I am somewhat concerned about this as well.  Unless a value is identified as a
pointer the garbage collector will soon ignore it.

[remyoudompheng, 2013-05-14 23:47:53]

2013/5/15 Brad Fitzpatrick <bradfitz@golang.org>:
> Why does the memmove take uintptr instead of, say, *byte?

There's no particular reason (it's just less verbose for pointer
arithmetic), maybe byte* or unsafe.Pointer is better for precise GC?

[bradfitz, 2013-05-14 23:49:57]

Yes. Everybody needs to get into the habit now of never using uintptr for a
pointer, even if you think it should be safe.  It will become increasingly
less safe.



On Tue, May 14, 2013 at 4:47 PM, Rémy Oudompheng
<remyoudompheng@gmail.com>wrote:

> 2013/5/15 Brad Fitzpatrick <bradfitz@golang.org>:
> > Why does the memmove take uintptr instead of, say, *byte?
>
> There's no particular reason (it's just less verbose for pointer
> arithmetic), maybe byte* or unsafe.Pointer is better for precise GC?
>

[cshapiro1, 2013-05-14 23:50:05]

On 2013/05/14 23:47:53, remyoudompheng wrote:
> There's no particular reason (it's just less verbose for pointer
> arithmetic), maybe byte* or unsafe.Pointer is better for precise GC?

byte* would be great, thanks.

[remyoudompheng, 2013-05-16 06:39:37]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, cshapiro@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-05-18 01:14:40]

Nice improvement on linux/arm (from pandaboard)

alarm(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                61           54  -11.67%
BenchmarkCopy2Byte                84           57  -31.84%
BenchmarkCopy4Byte               127           64  -49.53%
BenchmarkCopy8Byte               118           69  -41.10%
BenchmarkCopy12Byte              122           71  -41.48%
BenchmarkCopy16Byte              125           75  -39.84%
BenchmarkCopy32Byte              127           68  -45.75%
BenchmarkCopy128Byte             130          121   -6.92%
BenchmarkCopy1024Byte            468          437   -6.62%
BenchmarkCopy1String              67           52  -22.19%
BenchmarkCopy2String              71           57  -19.61%
BenchmarkCopy4String              81           63  -22.41%
BenchmarkCopy8String             130           67  -47.85%
BenchmarkCopy12String            108           70  -34.81%
BenchmarkCopy16String            111           74  -33.24%
BenchmarkCopy32String             85           69  -19.01%
BenchmarkCopy128String           117          121   +3.42%
BenchmarkCopy1024String          455          437   -3.96%

benchmark                   old MB/s     new MB/s  speedup
BenchmarkCopy1Byte             16.21        18.36    1.13x
BenchmarkCopy2Byte             23.58        34.59    1.47x
BenchmarkCopy4Byte             31.43        62.41    1.99x
BenchmarkCopy8Byte             67.72       115.03    1.70x
BenchmarkCopy12Byte            98.35       168.03    1.71x
BenchmarkCopy16Byte           127.33       212.71    1.67x
BenchmarkCopy32Byte           250.61       464.31    1.85x
BenchmarkCopy128Byte          979.61      1053.71    1.08x
BenchmarkCopy1024Byte        2185.20      2341.54    1.07x
BenchmarkCopy1String           14.78        19.01    1.29x
BenchmarkCopy2String           28.01        34.82    1.24x
BenchmarkCopy4String           49.24        63.54    1.29x
BenchmarkCopy8String           61.40       118.05    1.92x
BenchmarkCopy12String         110.15       170.53    1.55x
BenchmarkCopy16String         143.46       215.82    1.50x
BenchmarkCopy32String         375.80       464.09    1.23x
BenchmarkCopy128String       1086.87      1053.68    0.97x
BenchmarkCopy1024String      2246.60      2340.10    1.04x

alarm(~/go/src/pkg/runtime) % hg id
bce231eb0fdd+ tip

So this is after 4cb93e2900d0 was committed.

[remyoudompheng, 2013-05-18 08:15:51]

On 2013/05/18 01:14:40, dfc wrote:
> Nice improvement on linux/arm (from pandaboard)
> 
> alarm(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
> benchmark                  old ns/op    new ns/op    delta
> BenchmarkCopy1Byte                61           54  -11.67%
> BenchmarkCopy2Byte                84           57  -31.84%
> BenchmarkCopy4Byte               127           64  -49.53%
> BenchmarkCopy8Byte               118           69  -41.10%

Thanks, it looks as if function calls have a fixed cost of ~50ns.

[dave_cheney.net, 2013-05-18 08:20:17]

I'll try the same benchmark on my A15 chromebook later. Faster
processor and memory, but longer pipeline, should be interesting.

On Sat, May 18, 2013 at 6:15 PM,  <remyoudompheng@gmail.com> wrote:
> On 2013/05/18 01:14:40, dfc wrote:
>>
>> Nice improvement on linux/arm (from pandaboard)
>
>
>> alarm(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
>> benchmark                  old ns/op    new ns/op    delta
>> BenchmarkCopy1Byte                61           54  -11.67%
>> BenchmarkCopy2Byte                84           57  -31.84%
>> BenchmarkCopy4Byte               127           64  -49.53%
>> BenchmarkCopy8Byte               118           69  -41.10%
>
>
> Thanks, it looks as if function calls have a fixed cost of ~50ns.
>
>
> https://codereview.appspot.com/9101048/

[dave_cheney.net, 2013-05-18 23:58:34]

Here are results from linux/arm on a chromebook

localhost(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                21           19  -10.19%
BenchmarkCopy2Byte                33           26  -19.39%
BenchmarkCopy4Byte                30           19  -36.30%
BenchmarkCopy8Byte                32           21  -33.85%
BenchmarkCopy12Byte               33           23  -31.36%
BenchmarkCopy16Byte               34           24  -28.03%
BenchmarkCopy32Byte               35           22  -36.21%
BenchmarkCopy128Byte              46           35  -22.94%
BenchmarkCopy1024Byte            206          196   -4.85%
BenchmarkCopy1String              25           17  -32.55%
BenchmarkCopy2String              26           34  +30.04%
BenchmarkCopy4String              25           19  -25.49%
BenchmarkCopy8String              28           20  -26.07%
BenchmarkCopy12String             29           22  -23.99%
BenchmarkCopy16String             30           24  -19.27%
BenchmarkCopy32String             30           22  -26.86%
BenchmarkCopy128String            40           35  -13.37%
BenchmarkCopy1024String          201          195   -2.99%

benchmark                   old MB/s     new MB/s  speedup
BenchmarkCopy1Byte             46.28        51.64    1.12x
BenchmarkCopy2Byte             60.55        75.12    1.24x
BenchmarkCopy4Byte            131.88       207.16    1.57x
BenchmarkCopy8Byte            248.44       374.98    1.51x
BenchmarkCopy12Byte           355.25       517.70    1.46x
BenchmarkCopy16Byte           462.87       643.53    1.39x
BenchmarkCopy32Byte           891.13      1398.18    1.57x
BenchmarkCopy128Byte         2769.83      3593.05    1.30x
BenchmarkCopy1024Byte        4958.78      5204.99    1.05x
BenchmarkCopy1String           39.24        58.15    1.48x
BenchmarkCopy2String           76.10        58.42    0.77x
BenchmarkCopy4String          156.78       210.55    1.34x
BenchmarkCopy8String          285.38       386.19    1.35x
BenchmarkCopy12String         405.38       533.18    1.32x
BenchmarkCopy16String         531.86       659.42    1.24x
BenchmarkCopy32String        1036.41      1415.81    1.37x
BenchmarkCopy128String       3165.76      3657.31    1.16x
BenchmarkCopy1024String      5094.03      5244.11    1.03x

[dave_cheney.net, 2013-05-24 08:11:42]

ping

[dave_cheney.net, 2013-05-27 22:06:09]

On 2013/05/24 08:11:42, dfc wrote:
> ping

ping

[DMorsing, 2013-05-28 15:37:20]

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1576: n->type = types[TUNSAFEPTR];
Use TUINTPTR here. Using unsafe means that app engine wont be able to compile
anything using copy()

[rsc, 2013-06-03 19:56:30]

Please use unsafe.Pointer. That's what it's for. If we need to add some more
selective disabling of the u flag, that's fine.

[DMorsing, 2013-06-03 20:01:40]

On 2013/06/03 19:56:30, rsc wrote:
> Please use unsafe.Pointer. That's what it's for. If we need to add some more
> selective disabling of the u flag, that's fine.

I don't think we need to very selective. You could just disable it after
typechecking of the top level functions.

[remyoudompheng, 2013-06-04 07:03:39]

So it's already unsafe.Pointer. Is it okay that memmove uses *byte?

Also, the node "OSPTR" is not strictly necessary for the patch, but it makes
things easier to read and might be useful in other cases. It means S(lice|tring)
PoinTeR but a better name could be used if needed.

In naddr (?g/gsubr.c), it's written TUINTPTR, it is used for stack pointer maps?
I should change it to tptr to imitate what is done for ITAB.

[rsc, 2013-06-10 20:31:56]

Looks good mostly. Using *byte is fine for now.
In general the compiler will need to be careful not to use TUINTPTR for a
pointer. 

I'd like to know what's going on in gen.c though.

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/gen.c
File src/cmd/gc/gen.c (right):

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/gen.c#newcode299
src/cmd/gc/gen.c:299: case OCONVNOP: // FIXME.
This needs an explanation.

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1576: n->type = types[TUNSAFEPTR];
Does types[tptr] work, or does it cause a crash because it has no ->elem?

[DMorsing, 2013-06-21 18:06:01]

ping?

[rsc, 2013-06-27 03:17:54]

sorry, just overwhelmed by all the runtime stuff. will review tomorrow.

[rsc, 2013-06-27 15:41:36]

ha, this ping was for remy not for me. :-)

[bradfitz, 2013-07-23 16:37:37]

Remy, ping.

Do you plan to finish/submit this, or is it superseded by other CLs since?

[remyoudompheng, 2013-07-28 16:32:35]

On 2013/07/23 16:37:37, bradfitz wrote:
> Remy, ping.
> 
> Do you plan to finish/submit this, or is it superseded by other CLs since?

I still plan to finish this. I'll have a look at rsc's comments.

[remyoudompheng, 2013-08-18 14:41:19]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, cshapiro@google.com,
dave@cheney.net, daniel.morsing@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-08-18 14:42:50]

On 2013/07/28 16:32:35, remyoudompheng wrote:
> On 2013/07/23 16:37:37, bradfitz wrote:
> > Remy, ping.
> > 
> > Do you plan to finish/submit this, or is it superseded by other CLs since?
> 
> I still plan to finish this. I'll have a look at rsc's comments.

New benchmarks with Go 1.2 memmove:
benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                 9            4  -48.58%
BenchmarkCopy2Byte                13            9  -27.46%
BenchmarkCopy4Byte                13            8  -35.49%
BenchmarkCopy8Byte                13            8  -37.37%
BenchmarkCopy12Byte               14           11  -18.75%
BenchmarkCopy16Byte               14           11  -18.75%
BenchmarkCopy32Byte               19           14  -24.74%
BenchmarkCopy128Byte              31           26  -15.29%
BenchmarkCopy1024Byte             99           92   -6.87%
BenchmarkCopy1String              10            4  -60.28%
BenchmarkCopy2String              10            9  -16.57%
BenchmarkCopy4String              10            8  -20.56%
BenchmarkCopy8String              11            8  -24.64%
BenchmarkCopy12String             11           12   +8.40%
BenchmarkCopy16String             11           11   -5.88%
BenchmarkCopy32String             15           14   -6.62%
BenchmarkCopy128String            28           25   -9.15%
BenchmarkCopy1024String           95           93   -1.26%


Original benchmarks with Go 1.1 memmove (old CL description):
benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                10            4  -52.52%
BenchmarkCopy2Byte                36           26  -28.02%
BenchmarkCopy4Byte                74           55  -25.84%
BenchmarkCopy8Byte                36           26  -27.75%
BenchmarkCopy12Byte               74           55  -25.84%
BenchmarkCopy16Byte               36           26  -28.02%
BenchmarkCopy32Byte               36           26  -28.02%
BenchmarkCopy128Byte              38           28  -24.61%
BenchmarkCopy1024Byte            107           96  -10.28%
BenchmarkCopy1String              27            4  -84.11%
BenchmarkCopy2String              27           25   -4.44%
BenchmarkCopy4String              56           54   -3.02%
BenchmarkCopy8String              27           25   -4.81%
BenchmarkCopy12String             56           54   -3.02%
BenchmarkCopy16String             27           25   -4.44%
BenchmarkCopy32String             27           25   -5.17%
BenchmarkCopy128String            29           28   -4.05%
BenchmarkCopy1024String           96           96   -0.41%

[remyoudompheng, 2013-08-18 14:45:06]

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/gen.c
File src/cmd/gc/gen.c (right):

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/gen.c#newcode299
src/cmd/gc/gen.c:299: case OCONVNOP: // FIXME.
On 2013/06/10 20:31:57, rsc wrote:
> This needs an explanation.

removed.

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

https://codereview.appspot.com/9101048/diff/13001/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1576: n->type = types[TUNSAFEPTR];
On 2013/06/10 20:31:57, rsc wrote:
> Does types[tptr] work, or does it cause a crash because it has no ->elem?

It needs to be used for the memmove argument, and types[tptr] doesn't match *any
nor *byte. So I've changed to use the actual type (*byte for strings and *T for
[]T).

[khr, 2013-08-27 20:08:48]

https://codereview.appspot.com/9101048/diff/43001/src/cmd/5g/gsubr.c
File src/cmd/5g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/5g/gsubr.c#newcode1369
src/cmd/5g/gsubr.c:1369: a->etype = simtype[TUINTPTR];
Don't we also need the correct type here?

https://codereview.appspot.com/9101048/diff/43001/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/6g/gsubr.c#newcode1253
src/cmd/6g/gsubr.c:1253: a->etype = simtype[TUINTPTR];
...and here

https://codereview.appspot.com/9101048/diff/43001/src/cmd/8g/gsubr.c
File src/cmd/8g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/8g/gsubr.c#newcode2330
src/cmd/8g/gsubr.c:2330: a->etype = simtype[TUINTPTR];
...and here

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/go.h#newcode575
src/cmd/gc/go.h:575: OSPTR,  // data word of a slice or string as an uintptr
As a *T or *byte ?

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1587: if(t->etype == TSTRING)
The type is set correctly here.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2623
src/cmd/gc/walk.c:2623: //     a[0] = b[0]
What's the point of this side of the branch?  Optimizing 1-byte copy?  Avoiding
the multiply?  I worry that having the compare and branch wouldn't be worth
special-casing this.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2657
src/cmd/gc/walk.c:2657: l = list(l, nif);
Are there interesting cases where n is a constant?

[khr1, 2013-08-27 20:29:44]

By the way, do we really need OSPTR, or can we do the same with just OINDEX
with a constant zero index?


On Tue, Aug 27, 2013 at 1:08 PM, <khr@golang.org> wrote:

>
>
https://codereview.appspot.**com/9101048/diff/43001/src/**cmd/5g/gsubr.c<http...
> File src/cmd/5g/gsubr.c (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/5g/gsubr.c#newcode1369<https://codereview.appspot.com/9101048/diff/43001/src/cmd/5g/gsubr.c#newcode1369>
> src/cmd/5g/gsubr.c:1369: a->etype = simtype[TUINTPTR];
> Don't we also need the correct type here?
>
>
https://codereview.appspot.**com/9101048/diff/43001/src/**cmd/6g/gsubr.c<http...
> File src/cmd/6g/gsubr.c (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/6g/gsubr.c#newcode1253<https://codereview.appspot.com/9101048/diff/43001/src/cmd/6g/gsubr.c#newcode1253>
> src/cmd/6g/gsubr.c:1253: a->etype = simtype[TUINTPTR];
> ...and here
>
>
https://codereview.appspot.**com/9101048/diff/43001/src/**cmd/8g/gsubr.c<http...
> File src/cmd/8g/gsubr.c (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/8g/gsubr.c#newcode2330<https://codereview.appspot.com/9101048/diff/43001/src/cmd/8g/gsubr.c#newcode2330>
> src/cmd/8g/gsubr.c:2330: a->etype = simtype[TUINTPTR];
> ...and here
>
>
https://codereview.appspot.**com/9101048/diff/43001/src/**cmd/gc/go.h<https:/...
> File src/cmd/gc/go.h (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/gc/go.h#newcode575<https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/go.h#newcode575>
> src/cmd/gc/go.h:575: OSPTR,  // data word of a slice or string as an
> uintptr
> As a *T or *byte ?
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/gc/typecheck.c<https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c>
> File src/cmd/gc/typecheck.c (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/gc/typecheck.c#newcode1587<https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c#newcode1587>
> src/cmd/gc/typecheck.c:1587: if(t->etype == TSTRING)
> The type is set correctly here.
>
>
https://codereview.appspot.**com/9101048/diff/43001/src/**cmd/gc/walk.c<https...
> File src/cmd/gc/walk.c (right):
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/gc/walk.c#newcode2623<https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2623>
> src/cmd/gc/walk.c:2623: //     a[0] = b[0]
> What's the point of this side of the branch?  Optimizing 1-byte copy?
> Avoiding the multiply?  I worry that having the compare and branch
> wouldn't be worth special-casing this.
>
> https://codereview.appspot.**com/9101048/diff/43001/src/**
>
cmd/gc/walk.c#newcode2657<https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2657>
> src/cmd/gc/walk.c:2657: l = list(l, nif);
> Are there interesting cases where n is a constant?
>
>
https://codereview.appspot.**com/9101048/<https://codereview.appspot.com/9101...
>

[rsc, 2013-09-06 19:37:24]

https://codereview.appspot.com/9101048/diff/43001/src/cmd/5g/gsubr.c
File src/cmd/5g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/5g/gsubr.c#newcode1369
src/cmd/5g/gsubr.c:1369: a->etype = simtype[TUINTPTR];
On 2013/08/27 20:08:50, khr wrote:
> Don't we also need the correct type here?

No, simtype[] already reduces to just a Txxx value (TUINT32 or TUINT64).

https://codereview.appspot.com/9101048/diff/43001/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/6g/gsubr.c#newcode1253
src/cmd/6g/gsubr.c:1253: a->etype = simtype[TUINTPTR];
On 2013/08/27 20:08:50, khr wrote:
> ...and here

Same.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/8g/gsubr.c
File src/cmd/8g/gsubr.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/8g/gsubr.c#newcode2330
src/cmd/8g/gsubr.c:2330: a->etype = simtype[TUINTPTR];
On 2013/08/27 20:08:50, khr wrote:
> ...and here

Same.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2623
src/cmd/gc/walk.c:2623: //     a[0] = b[0]
On 2013/08/27 20:08:50, khr wrote:
> What's the point of this side of the branch?  Optimizing 1-byte copy? 
Avoiding
> the multiply?  I worry that having the compare and branch wouldn't be worth
> special-casing this.

I'm confused about this too.

[rsc, 2013-09-06 20:11:06]

On Tue, Aug 27, 2013 at 4:29 PM, Keith Randall <khr@google.com> wrote:

> By the way, do we really need OSPTR, or can we do the same with just
> OINDEX with a constant zero index?
>

You'd have to do OADDR of OINDEX with a constant zero index, and you'd have
to disable the bounds check in case len is 0, and you'd have to allow
taking the address of a string byte. Having OSPTR is probably worth it.

[remyoudompheng, 2013-09-07 13:07:21]

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/go.h#newcode575
src/cmd/gc/go.h:575: OSPTR,  // data word of a slice or string as an uintptr
On 2013/08/27 20:08:50, khr wrote:
> As a *T or *byte ?

I've tried to reword it.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c
File src/cmd/gc/typecheck.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/typecheck.c#newc...
src/cmd/gc/typecheck.c:1587: if(t->etype == TSTRING)
On 2013/08/27 20:08:50, khr wrote:
> The type is set correctly here.

What do you mean?

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2623
src/cmd/gc/walk.c:2623: //     a[0] = b[0]
On 2013/08/27 20:08:50, khr wrote:
> What's the point of this side of the branch?  Optimizing 1-byte copy? 
Avoiding
> the multiply?  I worry that having the compare and branch wouldn't be worth
> special-casing this.

Yes, it was because runtime.copy also did that. I've removed it.

https://codereview.appspot.com/9101048/diff/43001/src/cmd/gc/walk.c#newcode2657
src/cmd/gc/walk.c:2657: l = list(l, nif);
On 2013/08/27 20:08:50, khr wrote:
> Are there interesting cases where n is a constant?

n is the length of the destination slice so I don't think it can be a constant,
except maybe when to is a[:] and a is an array, but the simplification would
apply to many other places.

[remyoudompheng, 2013-09-07 13:07:41]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, cshapiro@google.com,
dave@cheney.net, daniel.morsing@gmail.com, rsc@golang.org, khr@golang.org,
khr@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-09-10 19:19:37]

On 2013/09/07 13:07:41, remyoudompheng wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org,
mailto:cshapiro@google.com,
> mailto:dave@cheney.net, mailto:daniel.morsing@gmail.com,
mailto:rsc@golang.org, mailto:khr@golang.org,
> mailto:khr@google.com (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

ping?

[remyoudompheng, 2013-09-11 19:45:23]

Do we want this in Go 1.2? The CL was created the day after Go 1.1 was
released...

[rsc, 2013-09-11 20:51:28]

LGTM

Yes, sorry. Just a bit behind.

[remyoudompheng, 2013-09-11 22:37:33]

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

cmd/gc: inline copy in frontend to call memmove directly.

A new node type OSPTR is added to refer to the data pointer of
strings and slices in a simple way during walk(). It will be
useful for future work on simplification of slice arithmetic.

benchmark                  old ns/op    new ns/op    delta
BenchmarkCopy1Byte                 9            8  -13.98%
BenchmarkCopy2Byte                14            8  -40.49%
BenchmarkCopy4Byte                13            8  -35.04%
BenchmarkCopy8Byte                13            8  -37.10%
BenchmarkCopy12Byte               14           12  -15.38%
BenchmarkCopy16Byte               14           12  -17.24%
BenchmarkCopy32Byte               19           14  -27.32%
BenchmarkCopy128Byte              31           26  -15.29%
BenchmarkCopy1024Byte            100           92   -7.50%
BenchmarkCopy1String              10            7  -28.99%
BenchmarkCopy2String              10            7  -28.06%
BenchmarkCopy4String              10            8  -22.69%
BenchmarkCopy8String              10            8  -23.30%
BenchmarkCopy12String             11           11   -5.88%
BenchmarkCopy16String             11           11   -5.08%
BenchmarkCopy32String             15           14   -6.58%
BenchmarkCopy128String            28           25  -10.60%
BenchmarkCopy1024String           95           95   +0.53%

R=golang-dev, bradfitz, cshapiro, dave, daniel.morsing, rsc, khr, khr
CC=golang-dev
https://codereview.appspot.com/9101048