[release-branch.go1.8] cmd/compile: don't move spills to loop exits where the spill is dead

We shouldn't move a spill to a loop exit where the spill itself
is dead.  The stack location assigned to the spill might already
be reused by another spill at this point.

The case we previously handled incorrectly is the one where the value
being spilled is still live, but the spill itself is dead.

Fixes #20472

Patching directly on the release branch because the spill moving code has
already been rewritten for 1.9. (And it doesn't have this bug.)

Change-Id: I26c5273dafd98d66ec448750073c2b354ef89ad6
Reviewed-on: https://go-review.googlesource.com/44033
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

3 files changed, 97 insertions, 2 deletions

diff --git a/src/cmd/compile/internal/ssa/export_test.go b/src/cmd/compile/internal/ssa/export_test.go
index ee6ed51d73..80fa92d94b 100644
--- a/src/cmd/compile/internal/ssa/export_test.go
+++ b/src/cmd/compile/internal/ssa/export_test.go
@@ -35,8 +35,20 @@ type DummyFrontend struct {
 func (DummyFrontend) StringData(s string) interface{} {
 	return nil
 }
-func (DummyFrontend) Auto(t Type) GCNode {
-	return nil
+
+type dummyGCNode struct {
+	typ  Type
+	name string
+}
+
+func (d *dummyGCNode) Typ() Type {
+	return d.typ
+}
+func (d *dummyGCNode) String() string {
+	return d.name
+}
+func (d DummyFrontend) Auto(t Type) GCNode {
+	return &dummyGCNode{typ: t, name: "dummy"}
 }
 func (d DummyFrontend) SplitString(s LocalSlot) (LocalSlot, LocalSlot) {
 	return LocalSlot{s.N, d.TypeBytePtr(), s.Off}, LocalSlot{s.N, d.TypeInt(), s.Off + 8}
diff --git a/src/cmd/compile/internal/ssa/regalloc.go b/src/cmd/compile/internal/ssa/regalloc.go
index 7bf778609e..90b5947f1c 100644
--- a/src/cmd/compile/internal/ssa/regalloc.go
+++ b/src/cmd/compile/internal/ssa/regalloc.go
@@ -1699,6 +1699,24 @@ sinking:
 			}
 			p := d.Preds[0].b // block in loop exiting to d.
 
+			// Check that the spill value is still live at the start of d.
+			// If it isn't, we can't move the spill here because some other value
+			// may be using the same stack slot.  See issue 20472.
+			// The spill value can't be defined in d, so that makes our lives easier.
+			for _, x := range stacklive[d.ID] {
+				if x == vsp.ID {
+					goto stillLive
+				}
+			}
+			for _, v := range d.Values {
+				if v.Op == OpLoadReg && v.Args[0] == vsp {
+					goto stillLive
+				}
+			}
+			// Spill is not live - abort sinking this spill.
+			continue sinking
+		stillLive:
+
 			endregs := s.endRegs[p.ID]
 			for _, regrec := range endregs {
 				if regrec.v == e && regrec.r != noRegister && regrec.c == e { // TODO: regrec.c != e implies different spill possible.
diff --git a/src/cmd/compile/internal/ssa/regalloc_test.go b/src/cmd/compile/internal/ssa/regalloc_test.go
index cf8f452d12..d3d1891bcd 100644
--- a/src/cmd/compile/internal/ssa/regalloc_test.go
+++ b/src/cmd/compile/internal/ssa/regalloc_test.go
@@ -31,3 +31,68 @@ func TestLiveControlOps(t *testing.T) {
 	regalloc(f.f)
 	checkFunc(f.f)
 }
+
+func TestSpillMove(t *testing.T) {
+	// Test for issue 20472. We shouldn't move a spill out to exit blocks
+	// if there is an exit block where the spill is dead but the pre-spill
+	// value is live.
+	c := testConfig(t)
+	ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr"} // dummy for testing
+	arg1Aux := c.fe.Auto(TypeInt64)
+	arg2Aux := c.fe.Auto(ptrType)
+	f := Fun(c, "entry",
+		Bloc("entry",
+			Valu("mem", OpInitMem, TypeMem, 0, nil),
+			Valu("x", OpArg, TypeInt64, 0, arg1Aux),
+			Valu("p", OpArg, ptrType, 0, arg2Aux),
+			Valu("a", OpAMD64TESTQ, TypeFlags, 0, nil, "x", "x"),
+			Goto("loop1"),
+		),
+		Bloc("loop1",
+			Valu("y", OpAMD64MULQ, TypeInt64, 0, nil, "x", "x"),
+			Eq("a", "loop2", "exit1"),
+		),
+		Bloc("loop2",
+			Eq("a", "loop1", "exit2"),
+		),
+		Bloc("exit1",
+			// store before call, y is available in a register
+			Valu("mem2", OpAMD64MOVQstore, TypeMem, 0, nil, "p", "y", "mem"),
+			Valu("mem3", OpAMD64CALLstatic, TypeMem, 0, nil, "mem2"),
+			Exit("mem3"),
+		),
+		Bloc("exit2",
+			// store after call, y must be loaded from a spill location
+			Valu("mem4", OpAMD64CALLstatic, TypeMem, 0, nil, "mem"),
+			Valu("mem5", OpAMD64MOVQstore, TypeMem, 0, nil, "p", "y", "mem4"),
+			Exit("mem5"),
+		),
+	)
+	flagalloc(f.f)
+	regalloc(f.f)
+	checkFunc(f.f)
+	// There should still be a spill in Loop1, and nowhere else.
+	if numSpills(f.blocks["loop1"]) != 1 {
+		t.Errorf("spill missing from loop1")
+	}
+	if numSpills(f.blocks["loop2"]) != 0 {
+		t.Errorf("spill present in loop2")
+	}
+	if numSpills(f.blocks["exit1"]) != 0 {
+		t.Errorf("spill present in exit1")
+	}
+	if numSpills(f.blocks["exit2"]) != 0 {
+		t.Errorf("spill present in exit2")
+	}
+
+}
+
+func numSpills(b *Block) int {
+	n := 0
+	for _, v := range b.Values {
+		if v.Op == OpStoreReg {
+			n++
+		}
+	}
+	return n
+}