[dev.ssa] cmd/compile: clean up nilcheck logic

Be more clear about the two conditions that we care about:
1) a block that performs a nil check (OpIsNonNil), which may be removed
2) a block that is the non-nil sucessor for an OpIsNonNil block

Now we only care about removing nilchecks for two scenarios:
- a type 1 block is dominated by a type 2 block for the same value
- a block is both type 1 and type 2 for the same value

Fixes math/big.

Change-Id: I50018a4014830461ddfe2a2daf588468e4a8f0b4
Reviewed-on: https://go-review.googlesource.com/14325
Reviewed-by: Keith Randall <khr@golang.org>

src/cmd/compile/internal/ssa/nilcheck.go

@@ -38,7 +38,7 @@ func nilcheckelim(f *Func) {
 	}
 
 	work := make([]bp, 0, 256)
-	work = append(work, bp{block: f.Entry, ptr: checkedptr(f.Entry)})
+	work = append(work, bp{block: f.Entry})
 
 	// map from value ID to bool indicating if value is known to be non-nil
 	// in the current dominator path being walked.  This slice is updated by
@@ -74,27 +74,38 @@ func nilcheckelim(f *Func) {
 		node := work[len(work)-1]
 		work = work[:len(work)-1]
 
-		var pushRecPtr bool
 		switch node.op {
 		case Work:
-			if node.ptr != nil {
-				// already have a nilcheck in the dominator path
-				if nonNilValues[node.ptr.ID] {
+			checked := checkedptr(node.block) // ptr being checked for nil/non-nil
+			nonnil := nonnilptr(node.block)   // ptr that is non-nil due to this blocks pred
+
+			if checked != nil {
+				// already have a nilcheck in the dominator path, or this block is a success
+				// block for the same value it is checking
+				if nonNilValues[checked.ID] || checked == nonnil {
 					// Eliminate the nil check.
 					// The deadcode pass will remove vestigial values,
 					// and the fuse pass will join this block with its successor.
 					node.block.Kind = BlockFirst
 					node.block.Control = nil
-				} else {
-					// new nilcheck so add a ClearPtr node to clear the
-					// ptr from the map of nil checks once we traverse
-					// back up the tree
-					work = append(work, bp{op: ClearPtr, ptr: node.ptr})
-					// and cause a new setPtr to be appended after the
-					// block's dominees
-					pushRecPtr = true
 				}
 			}
+
+			if nonnil != nil && !nonNilValues[nonnil.ID] {
+				// this is a new nilcheck so add a ClearPtr node to clear the
+				// ptr from the map of nil checks once we traverse
+				// back up the tree
+				work = append(work, bp{op: ClearPtr, ptr: nonnil})
+			}
+
+			// add all dominated blocks to the work list
+			for _, w := range domTree[node.block.ID] {
+				work = append(work, bp{block: w})
+			}
+
+			if nonnil != nil && !nonNilValues[nonnil.ID] {
+				work = append(work, bp{op: RecPtr, ptr: nonnil})
+			}
 		case RecPtr:
 			nonNilValues[node.ptr.ID] = true
 			continue
@@ -102,77 +113,6 @@ func nilcheckelim(f *Func) {
 			nonNilValues[node.ptr.ID] = false
 			continue
 		}
-
-		var nilBranch *Block
-		for _, w := range domTree[node.block.ID] {
-			// We are about to traverse down the 'ptr is nil' side
-			// of a nilcheck block, so save it for later.  This doesn't
-			// remove nil checks on the false side of the OpIsNonNil branch.
-			// This is important otherwise we would remove nil checks that
-			// are not redundant.
-			if node.block.Kind == BlockIf && node.block.Control.Op == OpIsNonNil &&
-				w == node.block.Succs[1] {
-				nilBranch = w
-				continue
-			}
-			work = append(work, bp{block: w, ptr: checkedptr(w)})
-		}
-
-		if nilBranch != nil {
-			// we pop from the back of the work slice, so this sets
-			// up the false branch to be operated on before the
-			// node.ptr is recorded
-			work = append(work, bp{op: RecPtr, ptr: node.ptr})
-			work = append(work, bp{block: nilBranch, ptr: checkedptr(nilBranch)})
-		} else if pushRecPtr {
-			work = append(work, bp{op: RecPtr, ptr: node.ptr})
-		}
-	}
-}
-
-// nilcheckelim0 is the original redundant nilcheck elimination algorithm.
-func nilcheckelim0(f *Func) {
-	// Exit early if there are no nil checks to eliminate.
-	var found bool
-	for _, b := range f.Blocks {
-		if checkedptr(b) != nil {
-			found = true
-			break
-		}
-	}
-	if !found {
-		return
-	}
-
-	// Eliminate redundant nil checks.
-	// A nil check is redundant if the same
-	// nil check has been performed by a
-	// dominating block.
-	// The efficacy of this pass depends
-	// heavily on the efficacy of the cse pass.
-	idom := dominators(f) // TODO: cache the dominator tree in the function, clearing when the CFG changes?
-	for _, b := range f.Blocks {
-		ptr := checkedptr(b)
-		if ptr == nil {
-			continue
-		}
-		var elim bool
-		// Walk up the dominator tree,
-		// looking for identical nil checks.
-		// TODO: This loop is O(n^2). See BenchmarkNilCheckDeep*.
-		for c := idom[b.ID]; c != nil; c = idom[c.ID] {
-			if checkedptr(c) == ptr {
-				elim = true
-				break
-			}
-		}
-		if elim {
-			// Eliminate the nil check.
-			// The deadcode pass will remove vestigial values,
-			// and the fuse pass will join this block with its successor.
-			b.Kind = BlockFirst
-			b.Control = nil
-		}
 	}
 }
 
@@ -184,3 +124,17 @@ func checkedptr(b *Block) *Value {
 	}
 	return nil
 }
+
+// nonnilptr returns the Value, if any,
+// that is non-nil due to b being the success block
+// of an OpIsNonNil block for the value and having a single
+// predecessor.
+func nonnilptr(b *Block) *Value {
+	if len(b.Preds) == 1 {
+		bp := b.Preds[0]
+		if bp.Kind == BlockIf && bp.Control.Op == OpIsNonNil && bp.Succs[0] == b {
+			return bp.Control.Args[0]
+		}
+	}
+	return nil
+}

src/cmd/compile/internal/ssa/nilcheck_test.go

@@ -382,3 +382,48 @@ func TestNilcheckUser(t *testing.T) {
 		}
 	}
 }
+
+// TestNilcheckBug reproduces a bug in nilcheckelim found by compiling math/big
+func TestNilcheckBug(t *testing.T) {
+	ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr"} // dummy for testing
+	c := NewConfig("amd64", DummyFrontend{t})
+	fun := Fun(c, "entry",
+		Bloc("entry",
+			Valu("mem", OpArg, TypeMem, 0, ".mem"),
+			Valu("sb", OpSB, TypeInvalid, 0, nil),
+			Goto("checkPtr")),
+		Bloc("checkPtr",
+			Valu("ptr1", OpConstPtr, ptrType, 0, nil, "sb"),
+			Valu("nilptr", OpConstNil, ptrType, 0, nil, "sb"),
+			Valu("bool1", OpNeqPtr, TypeBool, 0, nil, "ptr1", "nilptr"),
+			If("bool1", "secondCheck", "couldBeNil")),
+		Bloc("couldBeNil",
+			Goto("secondCheck")),
+		Bloc("secondCheck",
+			Valu("bool2", OpIsNonNil, TypeBool, 0, nil, "ptr1"),
+			If("bool2", "extra", "exit")),
+		Bloc("extra",
+			Goto("exit")),
+		Bloc("exit",
+			Exit("mem")))
+
+	CheckFunc(fun.f)
+	// we need the opt here to rewrite the user nilcheck
+	opt(fun.f)
+	nilcheckelim(fun.f)
+
+	// clean up the removed nil check
+	fuse(fun.f)
+	deadcode(fun.f)
+
+	CheckFunc(fun.f)
+	foundSecondCheck := false
+	for _, b := range fun.f.Blocks {
+		if b == fun.blocks["secondCheck"] && isNilCheck(b) {
+			foundSecondCheck = true
+		}
+	}
+	if !foundSecondCheck {
+		t.Errorf("secondCheck was eliminated, but shouldn't have")
+	}
+}

src/cmd/dist/test.go

@@ -281,9 +281,6 @@ func (t *tester) registerSSATest(pkg string) {
 	// known failures due to GOGC=off
 	case "runtime", "runtime/pprof", "runtime/trace", "sync":
 		return
-	// TODO: fix these failures
-	case "math/big", "cmd/compile/internal/big":
-		return
 	}
 	t.tests = append(t.tests, distTest{
 		name:    "go_test_ssa:" + pkg,