go.tools/go/ssa: write zero value when storing a compos...

go.tools/go/ssa: write zero value when storing a composite literal in-place if necessary

Previously, statements such as:

type T struct { a, b int }
[...]
x = T{}
x = T{b: 1}

would only affect the aggregate members mentioned in the composite
literal and leave the other members unchanged. This change causes us
to write a zero value to the target in cases where the target is not
already known to hold a zero value and the number of initializers in
the composite literal differs from the number of elements in its type.

[adonovan, 2014-04-25 14:07:56]

Thanks for diagnosing/fixing this.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go
File go/ssa/builder.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode414
go/ssa/builder.go:414: // value of expression e. If iszero is true, we can
assume that loc
s/we can assume/exprInPlace assumes/

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode421
go/ssa/builder.go:421: func (b *builder) exprInPlace(fn *Function, loc lvalue, e
ast.Expr, iszero bool) {
s/iszero/isZero/g

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1042
go/ssa/builder.go:1042: if !iszero && len(e.Elts) != t.NumFields() {
Depending on the difference between len(e.Elts) and t.NumFields(), zeroing the
entire object could be more expensive than adding additional field initializers.
 Do you have any sense of the relative cost, or whether a subsequent SRA pass
would make this moot?

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1043
go/ssa/builder.go:1043: emitStore(fn, addr, zeroValue(fn, typ))
Rather than allocate a local (which is implicitly zero-initialized), load it
into an ssa register, then store that to addr, I think we should probably define
a new intrinsic (ssa.Builtin) "ssa.memclr" which needs only a pointer.

This requires an API change since currently ssa.Builtin only represent those
defined by the spec, but this is a change I've wanted to make for a long time
for other intrinsics, such as "ssa.nilcheck".  I suggest replacing Builtin's
object field by  'string name', and making its Object() method return
types.Universe.Lookup(b.name).  You'll want a function like makeLen to return a
ssa.memclr Builtin of the appropriate pointer type.

If you think that's out of scope for a bug fix, just note it in a TODO at func
zeroValue for now.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1129
go/ssa/builder.go:1129: b.exprInPlace(fn, loc, e.Value, false)
Shouldn't this be true?  You just called makemap, and in any case map elements
are not addressable, so element.store() clobbers any existing element entirely.

Please add a testcase.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go
File go/ssa/emit.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go#newcode417
go/ssa/emit.go:417: func zeroValue(f *Function, t types.Type) Value {
This function belongs in emit.go.

Please add a TODO to replace this by an "ssa.memclr" Builtin.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
File go/ssa/interp/testdata/coverage.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:646: // Test for in-place initialization.
Good.

I'm surprised this wasn't covered by one of the existing $GOROOT/test cases,
which suggests perhaps this test should move into that tree so all toolchains
are exercised by it.

This coverage.go file is a bit of a dumping ground; I need to break it up and be
more systematic about corner cases at some point.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:648: type S struct {
// struct

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:667: type A [4]int
// array

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:694: }
Can you add testcases for slice and map?

[pcc, 2014-04-25 21:45:33]

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go
File go/ssa/builder.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode414
go/ssa/builder.go:414: // value of expression e. If iszero is true, we can
assume that loc
On 2014/04/25 14:07:57, adonovan wrote:
> s/we can assume/exprInPlace assumes/

Done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode421
go/ssa/builder.go:421: func (b *builder) exprInPlace(fn *Function, loc lvalue, e
ast.Expr, iszero bool) {
On 2014/04/25 14:07:57, adonovan wrote:
> s/iszero/isZero/g

Done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1042
go/ssa/builder.go:1042: if !iszero && len(e.Elts) != t.NumFields() {
On 2014/04/25 14:07:57, adonovan wrote:
> Depending on the difference between len(e.Elts) and t.NumFields(), zeroing the
> entire object could be more expensive than adding additional field
initializers.
>  Do you have any sense of the relative cost, or whether a subsequent SRA pass
> would make this moot?

I haven't measured it in my compiler, as we're currently more focused on
correctness than optimization. I think that if we do implement this
optimization, it needs to be done here rather than downstream, as only go/ssa
has visibility of the evaluation order constraints (or rather, the lack of them)
imposed by a composite literal assignment (a consumer of the go/ssa IR may have
to conservatively assume that a composite literal initializer may access the
aggregate being initialized, potentially preventing SROA or other
optimizations).

But in any case, this should wait until after the thaw.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1043
go/ssa/builder.go:1043: emitStore(fn, addr, zeroValue(fn, typ))
On 2014/04/25 14:07:57, adonovan wrote:
> Rather than allocate a local (which is implicitly zero-initialized), load it
> into an ssa register, then store that to addr, I think we should probably
define
> a new intrinsic (ssa.Builtin) "ssa.memclr" which needs only a pointer.

The other obvious approach would be to extend Const with the ability to
represent zero-value aggregates. (That could also be useful in other cases, such
as parameter and result passing.) Do you see any downsides to that approach?

> If you think that's out of scope for a bug fix, just note it in a TODO at func
> zeroValue for now.

Added TODO in zeroValue.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1129
go/ssa/builder.go:1129: b.exprInPlace(fn, loc, e.Value, false)
On 2014/04/25 14:07:57, adonovan wrote:
> Shouldn't this be true?  You just called makemap, and in any case map elements
> are not addressable, so element.store() clobbers any existing element
entirely.
> 
> Please add a testcase.

I was going back and forth on whether this should be false or true. I thought
that making it false may defend against any future changes to go/ssa which might
make map elements addressable at the IR level, as it is possible to construct a
composite literal which refers to the same map element multiple times, e.g.:

package main

func main() {
	s := "foo"
	m := map[string]struct{a, b int}{
		s: {a: 1},
		s: {b: 2},
	}
	println(m[s].a, m[s].b)
}

Although gccgo and gc print "0 2" for this example (seemingly indicating
initialization in lexical order), after looking through the spec I cannot find
anything which speaks to whether this is well defined, so it may well be fine to
change this to true regardless -- done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go
File go/ssa/emit.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go#newcode417
go/ssa/emit.go:417: func zeroValue(f *Function, t types.Type) Value {
On 2014/04/25 14:07:57, adonovan wrote:
> This function belongs in emit.go.

It *is* in emit.go.

> Please add a TODO to replace this by an "ssa.memclr" Builtin.

Done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
File go/ssa/interp/testdata/coverage.go (right):

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:648: type S struct {
On 2014/04/25 14:07:57, adonovan wrote:
> // struct

Done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:667: type A [4]int
On 2014/04/25 14:07:57, adonovan wrote:
> // array

Done.

https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
go/ssa/interp/testdata/coverage.go:694: }
On 2014/04/25 14:07:57, adonovan wrote:
> Can you add testcases for slice and map?

Would new test cases for slice/map in the style of these struct/array test cases
make sense? As far as I know, these wouldn't perform in-place initialization of
non-zero values so there would be no observable behavior change.

I imagine that a sufficiently smart go/ssa may be able to figure out when it
would be safe and beneficial to rewrite the elements of an existing slice/map
when a literal is assigned to it, but maybe those tests should be added with the
smarts.

[pcc, 2014-05-28 23:34:46]

On 2014/04/25 21:45:33, pcc wrote:
> https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go
> File go/ssa/builder.go (right):
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode414
> go/ssa/builder.go:414: // value of expression e. If iszero is true, we can
> assume that loc
> On 2014/04/25 14:07:57, adonovan wrote:
> > s/we can assume/exprInPlace assumes/
> 
> Done.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode421
> go/ssa/builder.go:421: func (b *builder) exprInPlace(fn *Function, loc lvalue,
e
> ast.Expr, iszero bool) {
> On 2014/04/25 14:07:57, adonovan wrote:
> > s/iszero/isZero/g
> 
> Done.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1042
> go/ssa/builder.go:1042: if !iszero && len(e.Elts) != t.NumFields() {
> On 2014/04/25 14:07:57, adonovan wrote:
> > Depending on the difference between len(e.Elts) and t.NumFields(), zeroing
the
> > entire object could be more expensive than adding additional field
> initializers.
> >  Do you have any sense of the relative cost, or whether a subsequent SRA
pass
> > would make this moot?
> 
> I haven't measured it in my compiler, as we're currently more focused on
> correctness than optimization. I think that if we do implement this
> optimization, it needs to be done here rather than downstream, as only go/ssa
> has visibility of the evaluation order constraints (or rather, the lack of
them)
> imposed by a composite literal assignment (a consumer of the go/ssa IR may
have
> to conservatively assume that a composite literal initializer may access the
> aggregate being initialized, potentially preventing SROA or other
> optimizations).
> 
> But in any case, this should wait until after the thaw.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1043
> go/ssa/builder.go:1043: emitStore(fn, addr, zeroValue(fn, typ))
> On 2014/04/25 14:07:57, adonovan wrote:
> > Rather than allocate a local (which is implicitly zero-initialized), load it
> > into an ssa register, then store that to addr, I think we should probably
> define
> > a new intrinsic (ssa.Builtin) "ssa.memclr" which needs only a pointer.
> 
> The other obvious approach would be to extend Const with the ability to
> represent zero-value aggregates. (That could also be useful in other cases,
such
> as parameter and result passing.) Do you see any downsides to that approach?
> 
> > If you think that's out of scope for a bug fix, just note it in a TODO at
func
> > zeroValue for now.
> 
> Added TODO in zeroValue.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/builder.go#newcode1129
> go/ssa/builder.go:1129: b.exprInPlace(fn, loc, e.Value, false)
> On 2014/04/25 14:07:57, adonovan wrote:
> > Shouldn't this be true?  You just called makemap, and in any case map
elements
> > are not addressable, so element.store() clobbers any existing element
> entirely.
> > 
> > Please add a testcase.
> 
> I was going back and forth on whether this should be false or true. I thought
> that making it false may defend against any future changes to go/ssa which
might
> make map elements addressable at the IR level, as it is possible to construct
a
> composite literal which refers to the same map element multiple times, e.g.:
> 
> package main
> 
> func main() {
> 	s := "foo"
> 	m := map[string]struct{a, b int}{
> 		s: {a: 1},
> 		s: {b: 2},
> 	}
> 	println(m[s].a, m[s].b)
> }
> 
> Although gccgo and gc print "0 2" for this example (seemingly indicating
> initialization in lexical order), after looking through the spec I cannot find
> anything which speaks to whether this is well defined, so it may well be fine
to
> change this to true regardless -- done.
> 
> https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go
> File go/ssa/emit.go (right):
> 
> https://codereview.appspot.com/92780043/diff/20001/go/ssa/emit.go#newcode417
> go/ssa/emit.go:417: func zeroValue(f *Function, t types.Type) Value {
> On 2014/04/25 14:07:57, adonovan wrote:
> > This function belongs in emit.go.
> 
> It *is* in emit.go.
> 
> > Please add a TODO to replace this by an "ssa.memclr" Builtin.
> 
> Done.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
> File go/ssa/interp/testdata/coverage.go (right):
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
> go/ssa/interp/testdata/coverage.go:648: type S struct {
> On 2014/04/25 14:07:57, adonovan wrote:
> > // struct
> 
> Done.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
> go/ssa/interp/testdata/coverage.go:667: type A [4]int
> On 2014/04/25 14:07:57, adonovan wrote:
> > // array
> 
> Done.
> 
>
https://codereview.appspot.com/92780043/diff/20001/go/ssa/interp/testdata/cov...
> go/ssa/interp/testdata/coverage.go:694: }
> On 2014/04/25 14:07:57, adonovan wrote:
> > Can you add testcases for slice and map?
> 
> Would new test cases for slice/map in the style of these struct/array test
cases
> make sense? As far as I know, these wouldn't perform in-place initialization
of
> non-zero values so there would be no observable behavior change.
> 
> I imagine that a sufficiently smart go/ssa may be able to figure out when it
> would be safe and beneficial to rewrite the elements of an existing slice/map
> when a literal is assigned to it, but maybe those tests should be added with
the
> smarts.

Ping.