code review 9662046: cmd/gc: use escape analysis result for make([]T, constant

cmd/gc: use escape analysis result for make([]T, constant

Escape analysis already gives that the underlying array
does not escape but the result was ignored.

Fixes issue 5484.

[remyoudompheng, 2013-05-22 21:41:28]

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

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

[dave_cheney.net, 2013-05-22 21:44:34]

Thank you. Why did the CL not need any modifications to test/escape*.go ?



On 23/05/2013, at 7:41, 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: use escape analysis result for make([]T, constant
> 
> Escape analsyis already gives that the underlying array
> does not escape but the result was ignored.
> 
> Fixes issue 5484.
> 
> Please review this at https://codereview.appspot.com/9662046/
> 
> Affected files:
>  src/cmd/gc/walk.c
> 
> 
> Index: src/cmd/gc/walk.c
> ===================================================================
> --- a/src/cmd/gc/walk.c
> +++ b/src/cmd/gc/walk.c
> @@ -1246,18 +1246,35 @@
>        goto ret;
> 
>    case OMAKESLICE:
> -        // makeslice(t *Type, nel int64, max int64) (ary []any)
>        l = n->left;
>        r = n->right;
>        if(r == nil)
>            l = r = safeexpr(l, init);
>        t = n->type;
> -        fn = syslook("makeslice", 1);
> -        argtype(fn, t->type);            // any-1
> -        n = mkcall1(fn, n->type, init,
> -            typename(n->type),
> -            conv(l, types[TINT64]),
> -            conv(r, types[TINT64]));
> +        if(n->esc == EscNone
> +            && smallintconst(l) && smallintconst(r)
> +            && mpgetfix(r->val.u.xval) < (1ULL<<16) / t->type->width) {
> +            // var arr [r]T
> +            // n = arr[:l]
> +            t = aindex(r, t->type); // [r]T
> +            var = temp(t);
> +            a = nod(OAS, var, N); // zero temp
> +            typecheck(&a, Etop);
> +            *init = list(*init, a);
> +            r = nod(OSLICE, var, nod(OKEY, N, l)); // arr[:l]
> +            r = conv(r, n->type); // in case n->type is named.
> +            typecheck(&r, Erv);
> +            walkexpr(&r, init);
> +            n = r;
> +        } else {
> +            // makeslice(t *Type, nel int64, max int64) (ary []any)
> +            fn = syslook("makeslice", 1);
> +            argtype(fn, t->type);            // any-1
> +            n = mkcall1(fn, n->type, init,
> +                typename(n->type),
> +                conv(l, types[TINT64]),
> +                conv(r, types[TINT64]));
> +        }
>        goto ret;
> 
>    case ORUNESTR:
> 
> 
> -- 
> 
> ---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.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[remyoudompheng, 2013-05-22 21:44:45]

The occurences of the pattern in the standard library are the following:

pkg/compress/bzip2/bzip2.go:189: putting make([]bool, 256) on stack
pkg/go/ast/ast.go:105: putting make([]string, 0, 10) on stack
cmd/yacc/yacc.go:537: putting make([]int, RULEINC) on stack
pkg/net/interface_linux.go:185: putting make([]byte, IPv4len) on stack
pkg/net/interface_linux.go:218: putting make([]byte, IPv6len) on stack
pkg/net/dnsmsg.go:622: putting make(IP, IPv6len) on stack
pkg/net/textproto/reader.go:391: putting make([]byte, 128) on stack
pkg/net/mail/message.go:372: putting make([]byte, 0, 10) on stack
pkg/crypto/tls/handshake_server.go:325: putting make([]byte, 4) on stack
pkg/net/http/fcgi/child.go:231: putting make([]byte, 8) on stack
pkg/net/http/fcgi/fcgi.go:167: putting make([]byte, 8) on stack

[remyoudompheng, 2013-05-22 21:45:14]

On 2013/05/22 21:44:34, dfc wrote:
> Thank you. Why did the CL not need any modifications to test/escape*.go ?
> 

Escape analysis already works. The result is just ignored in code generation.

[dave_cheney.net, 2013-05-22 21:53:35]

Ok, that was my mistake, I didn't realise that the escape*.go tests were only
looking at the front end. 



On 23/05/2013, at 7:45, remyoudompheng@gmail.com wrote:

> On 2013/05/22 21:44:34, dfc wrote:
>> Thank you. Why did the CL not need any modifications to
> test/escape*.go ?
> 
> 
> Escape analysis already works. The result is just ignored in code
> generation.
> 
> 
> https://codereview.appspot.com/9662046/

[dave_cheney.net, 2013-05-23 04:48:52]

Thanks Rémy, I think there is also a typo in the CL description.

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

https://codereview.appspot.com/9662046/diff/4001/src/cmd/gc/walk.c#newcode1256
src/cmd/gc/walk.c:1256: && mpgetfix(r->val.u.xval) < (1ULL<<16) /
t->type->width) {
Magic number alert, could this be defined somewhere ?

[remyoudompheng, 2013-05-23 06:11:20]

(fixed typo in description)

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

https://codereview.appspot.com/9662046/diff/4001/src/cmd/gc/walk.c#newcode1256
src/cmd/gc/walk.c:1256: && mpgetfix(r->val.u.xval) < (1ULL<<16) /
t->type->width) {
On 2013/05/23 04:48:52, dfc wrote:
> Magic number alert, could this be defined somewhere ?

Maybe, but it would need a name. The constant is used in the ONEW case, it's the
size beyond which we ignore escape analysis and allocate on heap anyway.

[dave_cheney.net, 2013-05-23 07:53:05]

Hmm, I thought that 1ULL<<16 would be common constant, but there are
no other occurrences, and plenty of other uncommented 1ULL<< shifts.
Please ignore my request.

On Thu, May 23, 2013 at 4:11 PM,  <remyoudompheng@gmail.com> wrote:
> (fixed typo in description)
>
>
>
> https://codereview.appspot.com/9662046/diff/4001/src/cmd/gc/walk.c
> File src/cmd/gc/walk.c (right):
>
>
https://codereview.appspot.com/9662046/diff/4001/src/cmd/gc/walk.c#newcode1256
> src/cmd/gc/walk.c:1256: && mpgetfix(r->val.u.xval) < (1ULL<<16) /
> t->type->width) {
> On 2013/05/23 04:48:52, dfc wrote:
>>
>> Magic number alert, could this be defined somewhere ?
>
>
> Maybe, but it would need a name. The constant is used in the ONEW case,
> it's the size beyond which we ignore escape analysis and allocate on
> heap anyway.
>
> https://codereview.appspot.com/9662046/

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

ping

[DMorsing, 2013-05-28 06:21:04]

LGTM.

[remyoudompheng, 2013-05-29 21:42:26]

I will submit this in a few hours if no-one objects.

[remyoudompheng, 2013-05-30 06:32:14]

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

cmd/gc: use escape analysis result for make([]T, constant

Escape analysis already gives that the underlying array
does not escape but the result was ignored.

Fixes issue 5484.

R=golang-dev, dave, daniel.morsing
CC=golang-dev
https://codereview.appspot.com/9662046