code review 7093050: cmd/gc: inlining of variadic functions.

cmd/gc: inlining of variadic functions.

[remyoudompheng, 2013-01-12 07:42:03]

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

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

[remyoudompheng, 2013-01-12 07:43:09]

- It could be enabled by default if useful
- I have a test for this but not sure where to put it:

package main

func sum(a ...int) int {
	if len(a) == 0 {
		return 0
	}
	s := a[0] + a[1] + a[2]
	return s
}

func sum2(a int, b ...int) int {
	s := a + b[0] + b[1]
	return s
}

func N() (a, b, c int) { return 1, 2, 3 }

func main() {
	u, v, w := 1, 2, 3
	s := sum(u, v, w)
	if s != 6 {
		panic(s)
	}
	s = sum2(u, v, w)
	if s != 6 {
		panic(s)
	}

	s = sum(N())
	if s != 6 {
		panic(s)
	}
	s = sum2(N())
	if s != 6 {
		panic(s)
	}

	V := []int{1, 2, 3}
	s = sum(V...)
	if s != 6 {
		panic(s)
	}
	s = sum2(3, V...)
	if s != 6 {
		panic(s)
	}

	s = sum()
	if s != 0 {
		panic(s)
	}
}

[remyoudompheng, 2013-01-12 07:46:31]

The following functions in the std library are simple enough to be inlined with
this patch:

src/pkg/io/multi.go:30: should inline MultiReader but it's variadic
src/pkg/io/multi.go:54: should inline MultiWriter but it's variadic
src/pkg/text/template/parse/parse.go:112: should inline New but it's variadic
src/cmd/cgo/gcc.go:1021: should inline (*TypeRepr).Set but it's variadic
src/cmd/cgo/out.go:895: should inline c but it's variadic
src/pkg/html/template/html.go:255: should inline commentEscaper but it's
variadic

[kardia, 2013-01-13 06:48:26]

Would it be correct to say, this in-lines the functions but doesn't save an 
alloc for the slice?

Thanks,
-Daniel


On Friday, January 11, 2013 11:42:03 PM UTC-8, Rémy wrote:
>
> Reviewers: rsc, lvd2, golang-dev_googlegroups.com, 
>
> Message: 
> Hello r...@golang.org <javascript:>, l...@gmail.com <javascript:>, 
> golan...@googlegroups.com <javascript:> (cc: 
> golan...@googlegroups.com <javascript:>), 
>
> I'd like you to review this change to 
> https://go.googlecode.com/hg/ 
>
>
> Description: 
> cmd/gc: inlining of variadic functions. 
>
> Please review this at https://codereview.appspot.com/7093050/ 
>
> Affected files: 
>    M src/cmd/gc/inl.c 
>
>
>

[lvd2, 2013-01-13 09:21:28]

LGTM

very nice.

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

https://codereview.appspot.com/7093050/diff/4001/src/cmd/gc/inl.c#newcode26
src/cmd/gc/inl.c:26: //   - inline functions with ... args
done, no?  and didn't you fix the one below recently too?

[remyoudompheng, 2013-01-13 09:52:41]

On 2013/1/13 Daniel Theophanes <kardianos@gmail.com> wrote:
> Would it be correct to say, this in-lines the functions but doesn't save an
> alloc for the slice?
>
> Thanks,
> -Daniel

It works like a normal call. Thanks to escape analysis even in normal
variadic calls, it can happen that the slice data is on the stack.

Rémy.

[remyoudompheng, 2013-01-16 21:59:55]

Any other comment about: whether the test could be committed somewhere? whether
the inlining should be enabled by default or left at level 3?

[rsc, 2013-01-30 16:35:12]

LGTM

I would prefer this not be on by default. Thanks.

[remyoudompheng, 2013-01-30 21:21:07]

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

Please take another look.

[remyoudompheng, 2013-01-30 21:21:58]

I seem to have written garbage in the previous patch.

[rsc, 2013-01-31 01:49:20]

LGTM

[remyoudompheng, 2013-01-31 07:41:15]

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

cmd/gc: inlining of variadic functions.

R=rsc, lvd, golang-dev, kardianos
CC=golang-dev
https://codereview.appspot.com/7093050

[remyoudompheng, 2013-01-31 07:41:35]

https://codereview.appspot.com/7093050/diff/19002/src/cmd/gc/inl.c
File src/cmd/gc/inl.c (right):

https://codereview.appspot.com/7093050/diff/19002/src/cmd/gc/inl.c#newcode621
src/cmd/gc/inl.c:621: if(n->list->n->left->type->outtuple > 1)
had to correct a little typo here.