Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: gclocals differ between -c=1 and -c=16 #20250

Closed
josharian opened this issue May 4, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@josharian
Copy link
Contributor

commented May 4, 2017

Build package zombiezen.com/go/capnproto2/capnpc-go with -gcflags=-S and -gcflags="-S -c=16", and diff the output. Results:

< 	0x0048 00072 (/Users/josh/all/src/zombiezen.com/go/capnproto2/capnpc-go/capnpc-go.go:462)	FUNCDATA	$0, gclocals·7d924da83ae7ac262931be5a824429d6(SB)
---
> 	0x0048 00072 (/Users/josh/all/src/zombiezen.com/go/capnproto2/capnpc-go/capnpc-go.go:462)	FUNCDATA	$0, gclocals·7bde7237bfadac85f358010371743b27(SB)
97025c97025
< gclocals·7d924da83ae7ac262931be5a824429d6 SRODATA dupok size=36
---
> gclocals·7bde7237bfadac85f358010371743b27 SRODATA dupok size=36
97027,97028c97027,97028
< 	0x0010 00 06 00 06 87 06 01 06 01 06 00 06 01 06 01 06  ................
< 	0x0020 01 06 01 06                                      ....
---
> 	0x0010 84 06 84 06 87 06 85 06 85 06 84 06 85 06 85 06  ................
> 	0x0020 85 06 85 06                                      ....

Investigating now.

@josharian josharian added this to the Go1.9 milestone May 4, 2017

@josharian josharian self-assigned this May 4, 2017

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

Minimized:

package p

type T struct {
	s string
}

func f(a T) (e interface{}) {
	defer func() {
		e = a.s
	}()
	return nil
}
$ go tool compile -S x.go > c1 && go tool compile -S -c=16 x.go > c16 && diff c1 c16
9c9
< 	0x0021 00033 (x.go:7)	FUNCDATA	$0, gclocals·11c2ba55266e2ff26342e3af642a4d89(SB)
---
> 	0x0021 00033 (x.go:7)	FUNCDATA	$0, gclocals·96db7a2043a59034b870c4203409d146(SB)
16,18c16,18
< 	0x0042 00066 (x.go:10)	MOVQ	AX, 24(SP)
< 	0x0047 00071 (x.go:10)	MOVQ	"".a+64(SP), AX
< 	0x004c 00076 (x.go:10)	MOVQ	AX, 32(SP)
---
> 	0x0042 00066 (x.go:10)	MOVQ	"".a+64(SP), CX
> 	0x0047 00071 (x.go:10)	MOVQ	AX, 24(SP)
> 	0x004c 00076 (x.go:10)	MOVQ	CX, 32(SP)
48c48
< 	0x0040 24 38 48 89 44 24 18 48 8b 44 24 40 48 89 44 24  $8H.D$.H.D$@H.D$
---
> 	0x0040 24 38 48 8b 4c 24 40 48 89 44 24 18 48 89 4c 24  $8H.L$@H.D$.H.L$
190,191c190,191
< gclocals·11c2ba55266e2ff26342e3af642a4d89 SRODATA dupok size=10
< 	0x0000 02 00 00 00 04 00 00 00 01 0c                    ..........
---
> gclocals·96db7a2043a59034b870c4203409d146 SRODATA dupok size=10
> 	0x0000 02 00 00 00 04 00 00 00 01 0d                    ..........
@josharian

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

When -c=16, a is marked as addrtaken on entry to SSA when compiling f. When -c=1, it is not.

The Node for a is shared between f and f.func1. When walking f.func1, a becomes an argument to convT2EString, and thus it gets marked as addrtaken. When c=1, we compile f before this modification occurs; when c=16, we walk both functions before compiling either.

The good news, I guess, is that this is just a pessimization.

(This comment updated to reflect a better understanding of the problem.)

@gopherbot

This comment has been minimized.

Copy link

commented May 5, 2017

CL https://golang.org/cl/42853 mentions this issue.

josharian added a commit to josharian/go that referenced this issue May 10, 2017

cmd/compile: don't update outer variables after capturevars is complete
When compiling concurrently, we walk all functions before compiling
any of them. Walking functions can cause variables to switch from
being non-addrtaken to addrtaken, e.g. to prepare for a runtime call.
Typechecking propagates addrtaken-ness of closure variables to
their outer variables, so that capturevars can decide whether to
pass the variable's value or a pointer to it.

When all functions are compiled immediately, as long as the containing
function is compiled prior to the closure, this propagation has no effect.
When compilation is deferred, though, in rare cases, this results in 
a change in the addrtaken-ness of a variable in the outer function,
which in turn changes the compiler's output.
(This is rare because in a great many cases, a temporary has been
introduced, insulating the outer variable from modification.)
But concurrent compilation must generate identical results.

To fix this, track whether capturevars has run.
If it has, there is no need to update outer variables
when closure variables change.
Capturevars always runs before any functions are walked or compiled.

The remainder of the changes in this CL are to support the test.
In particular, -d=compilelater forces the compiler to walk all
functions before compiling any of them, despite being non-concurrent.
This is useful because -live is fundamentally incompatible with
concurrent compilation, but we want -c=1 to have no behavior changes.

Fixes golang#20250

Change-Id: I89bcb54268a41e8588af1ac8cc37fbef856a90c2

@gopherbot gopherbot closed this in 61336b7 May 14, 2017

@golang golang locked and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.