code review 216380043: compiler: Stack-allocate non-escaping variables.

compiler: Stack-allocate non-escaping variables.

This change allows variables initialized through make or new
to be allocated on the stack via a temporary variable if they
do not escape their function. It also improves the analysis to
consider situations where variables escape in the standard
library and go testsuite such as:

*nested composite literals and composite literal arguments
*method receivers always escaping
*escape via statements in closures referring to enclosing variables
*escape via calls with multiple return results

[cmang, 2015-03-26 21:30:53]

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

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

[cmang, 2015-03-26 21:32:33]

On 2015/03/26 21:30:53, cmang wrote:
> Hello mailto:iant@golang.org (cc: mailto:gofrontend-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/gofrontend

I'm still not sure about the discrepancies I'm seeing in the number of reported
allocations by runtime.ReadMemStats, but I'll add something about the % of
reduced allocations this resulted in once I am.

[iant, 2015-03-26 23:42:39]

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc
File go/escape.cc (right):

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode592
go/escape.cc:592: if (ce->fn()->var_expression() != NULL
Just below you have
    if (ce->fn()->func_expression() == NULL)
      return;

I think the conservative approach is to move that up here.  That is, replace the
test of ce->fn()->var_expression != NULL ||
ce->fn()->interface_field_reference_expression() != NULL with the single test
ce->fn()->func_expression() == NULL.  Then do the conservative analysis, then
return.

To put this another way, is there a case where var_expression() == NULL &&
interface_field_reference_expression() == NULL && func_expression() == NULL
where you do *not* want to treat every argument as escaping?

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode633
go/escape.cc:633: std::list<int> positions;
I think move the declaration of positions down below this loop, since it isn't
used until the following loop.

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode726
go/escape.cc:726: || fntype->is_builtin())
Add a TODO note to get the right escape info for builtin calls.  A call to a
builtin function like BYTE_ARRAY_TO_STRING does not cause anything to escape.

[cmang, 2015-03-26 23:48:55]

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc
File go/escape.cc (right):

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode592
go/escape.cc:592: if (ce->fn()->var_expression() != NULL
On 2015/03/26 23:42:39, iant wrote:
> Just below you have
>     if (ce->fn()->func_expression() == NULL)
>       return;
> 
> I think the conservative approach is to move that up here.  That is, replace
the
> test of ce->fn()->var_expression != NULL ||
> ce->fn()->interface_field_reference_expression() != NULL with the single test
> ce->fn()->func_expression() == NULL.  Then do the conservative analysis, then
> return.
> 
> To put this another way, is there a case where var_expression() == NULL &&
> interface_field_reference_expression() == NULL && func_expression() == NULL
> where you do *not* want to treat every argument as escaping?

Done.

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode633
go/escape.cc:633: std::list<int> positions;
On 2015/03/26 23:42:39, iant wrote:
> I think move the declaration of positions down below this loop, since it isn't
> used until the following loop.

Done.

https://codereview.appspot.com/216380043/diff/50001/go/escape.cc#newcode726
go/escape.cc:726: || fntype->is_builtin())
On 2015/03/26 23:42:39, iant wrote:
> Add a TODO note to get the right escape info for builtin calls.  A call to a
> builtin function like BYTE_ARRAY_TO_STRING does not cause anything to escape.

Done.

[iant, 2015-03-27 21:24:47]

Do you think this is basically ready to commit when these issues are addressed?

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc
File go/escape.cc (right):

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc#newcode795
go/escape.cc:795: if (position < 0)
This does not seem right.  It seems to me that if position < 0 can ever be true,
then we should always be incrementing position when looking in ce->args().  I
don't see how it could be correct to only increment it if it is less than zero. 
Perhaps we should always increment if fntype->is_method().  I haven't tried to
figure it out.

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc#newcode1214
go/escape.cc:1214: // The receiver of a method always escapes from the method.
This is only true for a pointer method, that is if
    fntype->receiver()->type()->points_to() != NULL

For a value method the receiver is copied when the method starts, so it does not
necessarily escape.  I'm not sure whether the code above will handle this case
correctly, though.

[cmang, 2015-03-31 15:04:59]

PTAL; I think this is ready to commit.

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc
File go/escape.cc (right):

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc#newcode795
go/escape.cc:795: if (position < 0)
On 2015/03/27 21:24:47, iant wrote:
> This does not seem right.  It seems to me that if position < 0 can ever be
true,
> then we should always be incrementing position when looking in ce->args().  I
> don't see how it could be correct to only increment it if it is less than
zero. 
> Perhaps we should always increment if fntype->is_method().  I haven't tried to
> figure it out.

You're right, it should be always incremented when fntype->is_method(). I was
doing this a different way before and didn't update this snippet.

https://codereview.appspot.com/216380043/diff/70001/go/escape.cc#newcode1214
go/escape.cc:1214: // The receiver of a method always escapes from the method.
On 2015/03/27 21:24:47, iant wrote:
> This is only true for a pointer method, that is if
>     fntype->receiver()->type()->points_to() != NULL
> 
> For a value method the receiver is copied when the method starts, so it does
not
> necessarily escape.  I'm not sure whether the code above will handle this case
> correctly, though.
> 

Done.

[iant, 2015-03-31 17:25:39]

LGTM

[iant, 2015-03-31 17:25:55]

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

compiler: Stack-allocate non-escaping variables.

This change allows variables initialized through make or new
to be allocated on the stack via a temporary variable if they
do not escape their function. It also improves the analysis to
consider situations where variables escape in the standard
library and go testsuite such as:

*nested composite literals and composite literal arguments
*method receivers always escaping
*escape via statements in closures referring to enclosing variables
*escape via calls with multiple return results

LGTM=iant
R=iant
CC=gofrontend-dev
https://codereview.appspot.com/216380043

Committer: Ian Lance Taylor <iant@golang.org>