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

Jump to bottom

cmd/compile: writebarrier code not happy with single(ish)-block-loops that contain writebarriers #19067

Closed
dr2chase opened this issue Feb 13, 2017 · 5 comments
Closed

cmd/compile: writebarrier code not happy with single(ish)-block-loops that contain writebarriers #19067

dr2chase opened this issue Feb 13, 2017 · 5 comments
Labels
FrozenDueToAge

Comments

@dr2chase
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9 devel:
go version devel +e2948f7efe Mon Feb 13 20:30:31 2017 +0000 darwin/amd64

What operating system and processor architecture are you using (go env)?

amd64/darwin

What did you do?

https://play.golang.org/p/72MnkjS4P4

What did you expect to see?

This works in the playground.

*pa[5]= 6

What did you see instead?

# command-line-arguments
./bar.go:9: internal compiler error: value v42 depends on WB store v44 in the same block b8

goroutine 1 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/.../go/src/runtime/debug/stack.go:24 +0x79
cmd/compile/internal/gc.Fatalf(0x16c3354, 0x34, 0xc420349890, 0x3, 0x3)
	/.../go/src/cmd/compile/internal/gc/subr.go:175 +0x230
cmd/compile/internal/gc.(*ssaExport).Fatalf(0x19bbd55, 0x90900000001, 0x16c3354, 0x34, 0xc420349890, 0x3, 0x3)
	/.../go/src/cmd/compile/internal/gc/ssa.go:4939 +0x67
cmd/compile/internal/ssa.(*Config).Fatalf(0xc42038c000, 0x90900000001, 0x16c3354, 0x34, 0xc420349890, 0x3, 0x3)
	/.../go/src/cmd/compile/internal/ssa/config.go:345 +0x76
cmd/compile/internal/ssa.(*Func).Fatalf(0xc42036fd40, 0x16c3354, 0x34, 0xc420349890, 0x3, 0x3)
	/.../go/src/cmd/compile/internal/ssa/func.go:416 +0x72
cmd/compile/internal/ssa.writebarrier(0xc42036fd40)
	/.../go/src/cmd/compile/internal/ssa/writebarrier.go:106 +0xb3f
cmd/compile/internal/ssa.Compile(0xc42036fd40)
	/.../go/src/cmd/compile/internal/ssa/compile.go:70 +0x2c4
cmd/compile/internal/gc.buildssa(0xc4200c3180, 0x0)
	/.../go/src/cmd/compile/internal/gc/ssa.go:181 +0x10d7
cmd/compile/internal/gc.compile(0xc4200c3180)
	/.../go/src/cmd/compile/internal/gc/pgen.go:366 +0x2d3
cmd/compile/internal/gc.funccompile(0xc4200c3180)
	/.../go/src/cmd/compile/internal/gc/dcl.go:1226 +0xdc
cmd/compile/internal/gc.Main()
	/.../go/src/cmd/compile/internal/gc/main.go:473 +0x203c
main.main()
	/.../go/src/cmd/compile/main.go:50 +0xfe

There's a check in the writebarrier code that might not be necessary, or might need to exclude phi functions:

// make sure that no value in this block depends on WB stores
for _, w := range b.Values {
	if w.Op == OpStoreWB || w.Op == OpMoveWB || w.Op == OpMoveWBVolatile || w.Op == OpZeroWB {
		continue
	}
	for _, a := range w.Args {
		if wbs.contains(a.ID) {
			f.Fatalf("value %v depends on WB store %v in the same block %v", w, a, b)
		}
	}
}

The offending input looks like this:

b8: ← b6 b10
v42 = Phi <mem> v17 v44
v54 = Phi <int> v8 v46
v43 = NilCheck <void> v26 v42
v44 = StoreWB <mem> [8] v26 v36 v42
Plain → b9

b9: ← b8
v35 = Const64 <int> [1]
v46 = Add64 <int> v35 v54
v48 = Less64 <bool> v46 v53
If v48 → b10 b11

b10: ← b9
Plain → b8
@cherrymui
Copy link
Member

I have mailed a sequence of CLs for the new writebarrier implementation this morning. The new code doesn't have this problem.

@cherrymui
Copy link
Member

In particular, CL https://go-review.googlesource.com/c/36834/ is the one most relevant to this issue.

@cherrymui
Copy link
Member

And if we were to fix the old implementation, yes, Phi function needs to be special-cased in that check.

@josharian josharian changed the title writebarrier code not happy with single(ish)-block-loops that contain writebarriers cmd/compile: writebarrier code not happy with single(ish)-block-loops that contain writebarriers Feb 14, 2017
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 16, 2017
@cherrymui
cmd/compile: add a test for writebarrier pass with single-block loop
d75925d 
The old writebarrier implementation fails to handle single-block
loop where a memory Phi value depends on the write barrier store
in the same block. The new implementation (CL 36834) doesn't have
this problem. Add a test to ensure it.

Fix #19067.

Change-Id: Iab13c6817edc12be8a048d18699b4450fa7ed712
Reviewed-on: https://go-review.googlesource.com/36940
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Feb 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dr2chase @gopherbot @cherrymui