go/ssa/interp: assign phi nodes in parallel

This CL causes the interpreter to assign all phi nodes of a block
in parallel, as required by SSA semantics. Previously it would
execute them in order like real instructions, so that earlier
phis might clobber the environment values required as inputs
to later phis.

The phi handling is moved out of the ordinary visitInstr code
and into the block-entry logic, since all phis need to be
handled en bloc.

Also, a test, based on the user-reported problem in the attached
issue.

Fixes golang/go#69929

Change-Id: I12a61286b2151e2e72b642ca336e4ae31b7fa614
Reviewed-on: https://go-review.googlesource.com/c/tools/+/621595
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Tim King <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Commit-Queue: Tim King <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/ssa/interp/interp.go

@@ -48,9 +48,11 @@ import (
 	"fmt"
 	"go/token"
 	"go/types"
+	"log"
 	"os"
 	"reflect"
 	"runtime"
+	"slices"
 	"sync/atomic"
 	_ "unsafe"
 
@@ -108,6 +110,7 @@ type frame struct {
 	result           value
 	panicking        bool
 	panic            interface{}
+	phitemps         []value // temporaries for parallel phi assignment
 }
 
 func (fr *frame) get(key ssa.Value) value {
@@ -379,12 +382,7 @@ func visitInstr(fr *frame, instr ssa.Instruction) continuation {
 		fr.env[instr] = &closure{instr.Fn.(*ssa.Function), bindings}
 
 	case *ssa.Phi:
-		for i, pred := range instr.Block().Preds {
-			if fr.prevBlock == pred {
-				fr.env[instr] = fr.get(instr.Edges[i])
-				break
-			}
-		}
+		log.Fatal("unreachable") // phis are processed at block entry
 
 	case *ssa.Select:
 		var cases []reflect.SelectCase
@@ -589,25 +587,57 @@ func runFrame(fr *frame) {
 		if fr.i.mode&EnableTracing != 0 {
 			fmt.Fprintf(os.Stderr, ".%s:\n", fr.block)
 		}
-	block:
-		for _, instr := range fr.block.Instrs {
+
+		nonPhis := executePhis(fr)
+		for _, instr := range nonPhis {
 			if fr.i.mode&EnableTracing != 0 {
 				if v, ok := instr.(ssa.Value); ok {
 					fmt.Fprintln(os.Stderr, "\t", v.Name(), "=", instr)
 				} else {
 					fmt.Fprintln(os.Stderr, "\t", instr)
 				}
 			}
-			switch visitInstr(fr, instr) {
-			case kReturn:
+			if visitInstr(fr, instr) == kReturn {
 				return
-			case kNext:
-				// no-op
-			case kJump:
-				break block
 			}
+			// Inv: kNext (continue) or kJump (last instr)
+		}
+	}
+}
+
+// executePhis executes the phi-nodes at the start of the current
+// block and returns the non-phi instructions.
+func executePhis(fr *frame) []ssa.Instruction {
+	firstNonPhi := -1
+	for i, instr := range fr.block.Instrs {
+		if _, ok := instr.(*ssa.Phi); !ok {
+			firstNonPhi = i
+			break
+		}
+	}
+	// Inv: 0 <= firstNonPhi; every block contains a non-phi.
+
+	nonPhis := fr.block.Instrs[firstNonPhi:]
+	if firstNonPhi > 0 {
+		phis := fr.block.Instrs[:firstNonPhi]
+		// Execute parallel assignment of phis.
+		//
+		// See "the swap problem" in Briggs et al's "Practical Improvements
+		// to the Construction and Destruction of SSA Form" for discussion.
+		predIndex := slices.Index(fr.block.Preds, fr.prevBlock)
+		fr.phitemps = fr.phitemps[:0]
+		for _, phi := range phis {
+			phi := phi.(*ssa.Phi)
+			if fr.i.mode&EnableTracing != 0 {
+				fmt.Fprintln(os.Stderr, "\t", phi.Name(), "=", phi)
+			}
+			fr.phitemps = append(fr.phitemps, fr.get(phi.Edges[predIndex]))
+		}
+		for i, phi := range phis {
+			fr.env[phi.(*ssa.Phi)] = fr.phitemps[i]
 		}
 	}
+	return nonPhis
 }
 
 // doRecover implements the recover() built-in.

go/ssa/interp/interp_test.go

@@ -136,6 +136,7 @@ var testdataTests = []string{
 	"fixedbugs/issue52835.go",
 	"fixedbugs/issue55086.go",
 	"fixedbugs/issue66783.go",
+	"fixedbugs/issue69929.go",
 	"typeassert.go",
 	"zeros.go",
 	"slice2array.go",

go/ssa/interp/testdata/fixedbugs/issue69929.go

@@ -0,0 +1,67 @@
+package main
+
+// This is a regression test for a bug (#69929) in
+// the SSA interpreter in which it would not execute phis in parallel.
+//
+// The insert function below has interdependent phi nodes:
+//
+//	  entry:
+//		t0 = *root       // t0 is x or y before loop
+//		jump test
+//	  body:
+//		print(t5)      // t5 is x at loop entry
+//		t3 = t5.Child    // t3 is x after loop
+//		jump test
+//	  test:
+//		t5 = phi(t0, t3) // t5 is x at loop entry
+//		t6 = phi(t0, t5) // t6 is y at loop entry
+//		if t5 != nil goto body else done
+//	  done:
+//		print(t6)
+//		return
+//
+// The two phis:
+//
+//	t5 = phi(t0, t3)
+//	t6 = phi(t0, t5)
+//
+// must be executed in parallel as if they were written in Go
+// as:
+//
+//	t5, t6 = phi(t0, t3), phi(t0, t5)
+//
+// with the second phi node observing the original, not
+// updated, value of t5. (In more complex examples, the phi
+// nodes may be mutually recursive, breaking partial solutions
+// based on simple reordering of the phi instructions. See the
+// Briggs paper for detail.)
+//
+// The correct behavior is print(1, root); print(2, root); print(3, root).
+// The previous incorrect behavior had print(2, nil).
+
+func main() {
+	insert()
+	print(3, root)
+}
+
+var root = new(node)
+
+type node struct{ child *node }
+
+func insert() {
+	x := root
+	y := x
+	for x != nil {
+		y = x
+		print(1, y)
+		x = x.child
+	}
+	print(2, y)
+}
+
+func print(order int, ptr *node) {
+	println(order, ptr)
+	if ptr != root {
+		panic(ptr)
+	}
+}