cmd/compile: don't use nilcheck information until the next block

When nilcheck runs, the values in a block are not in any particular
order.  So any facts derived from examining the blocks shouldn't be
used until we reach the next block.

This is suboptimal as it won't eliminate nil checks within a block.
But it's probably a better fix for now as it is a much smaller change
than other strategies for fixing this bug.

nilptr3.go changes are mostly because for this pattern:
  _ = *p
  _ = *p
either nil check is fine to keep, and this CL changes which one
the compiler tends to keep.
There are a few regressions from code like this:
  _ = *p
  f()
  _ = *p
For this pattern, after this CL we issue 2 nil checks instead of one.
(For the curious, this happens because intra-block nil check
 elimination now falls to CSE, not nilcheck proper.  The former
 pattern has two nil checks with the same store argument.  The latter
 pattern has two nil checks with different store arguments.)

Fixes #18725

Change-Id: I3721b494c8bc9ba1142dc5c4361ea55c66920ac8
Reviewed-on: https://go-review.googlesource.com/35485
Reviewed-by: Cherry Zhang <cherryyz@google.com>

3 files changed, 59 insertions, 23 deletions

diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go
index 9f58db664b..0a34cd1ae6 100644
--- a/src/cmd/compile/internal/ssa/nilcheck.go
+++ b/src/cmd/compile/internal/ssa/nilcheck.go
@@ -82,7 +82,7 @@ func nilcheckelim(f *Func) {
 				}
 			}
 
-			// Next, process values in the block.
+			// Next, eliminate any redundant nil checks in this block.
 			i := 0
 			for _, v := range b.Values {
 				b.Values[i] = v
@@ -105,13 +105,10 @@ func nilcheckelim(f *Func) {
 							f.Config.Warnl(v.Line, "removed nil check")
 						}
 						v.reset(OpUnknown)
+						// TODO: f.freeValue(v)
 						i--
 						continue
 					}
-					// Record the fact that we know ptr is non nil, and remember to
-					// undo that information when this dominator subtree is done.
-					nonNilValues[ptr.ID] = true
-					work = append(work, bp{op: ClearPtr, ptr: ptr})
 				}
 			}
 			for j := i; j < len(b.Values); j++ {
@@ -119,6 +116,21 @@ func nilcheckelim(f *Func) {
 			}
 			b.Values = b.Values[:i]
 
+			// Finally, find redundant nil checks for subsequent blocks.
+			// Note that we can't add these until the loop above is done, as the
+			// values in the block are not ordered in any way when this pass runs.
+			// This was the cause of issue #18725.
+			for _, v := range b.Values {
+				if v.Op != OpNilCheck {
+					continue
+				}
+				ptr := v.Args[0]
+				// Record the fact that we know ptr is non nil, and remember to
+				// undo that information when this dominator subtree is done.
+				nonNilValues[ptr.ID] = true
+				work = append(work, bp{op: ClearPtr, ptr: ptr})
+			}
+
 			// Add all dominated blocks to the work list.
 			for w := sdom[node.block.ID].child; w != nil; w = sdom[w.ID].sibling {
 				work = append(work, bp{op: Work, block: w})
diff --git a/test/fixedbugs/issue18725.go b/test/fixedbugs/issue18725.go
new file mode 100644
index 0000000000..c632dbad63
--- /dev/null
+++ b/test/fixedbugs/issue18725.go
@@ -0,0 +1,24 @@
+// run
+
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "os"
+
+func panicWhenNot(cond bool) {
+	if cond {
+		os.Exit(0)
+	} else {
+		panic("nilcheck elim failed")
+	}
+}
+
+func main() {
+	e := (*string)(nil)
+	panicWhenNot(e == e)
+	// Should never reach this line.
+	panicWhenNot(*e == *e)
+}
diff --git a/test/nilptr3.go b/test/nilptr3.go
index 8fdae8c075..c681cba50c 100644
--- a/test/nilptr3.go
+++ b/test/nilptr3.go
@@ -40,23 +40,23 @@ var (
 )
 
 func f1() {
-	_ = *intp // ERROR "generated nil check"
+	_ = *intp // ERROR "removed nil check"
 
 	// This one should be removed but the block copy needs
 	// to be turned into its own pseudo-op in order to see
 	// the indirect.
-	_ = *arrayp // ERROR "generated nil check"
+	_ = *arrayp // ERROR "removed nil check"
 
 	// 0-byte indirect doesn't suffice.
 	// we don't registerize globals, so there are no removed.* nil checks.
-	_ = *array0p // ERROR "generated nil check"
 	_ = *array0p // ERROR "removed nil check"
+	_ = *array0p // ERROR "generated nil check"
 
-	_ = *intp    // ERROR "removed nil check"
+	_ = *intp    // ERROR "generated nil check"
 	_ = *arrayp  // ERROR "removed nil check"
 	_ = *structp // ERROR "generated nil check"
 	_ = *emptyp  // ERROR "generated nil check"
-	_ = *arrayp  // ERROR "removed nil check"
+	_ = *arrayp  // ERROR "generated nil check"
 }
 
 func f2() {
@@ -71,15 +71,15 @@ func f2() {
 		empty1p    *Empty1
 	)
 
-	_ = *intp       // ERROR "generated nil check"
-	_ = *arrayp     // ERROR "generated nil check"
-	_ = *array0p    // ERROR "generated nil check"
-	_ = *array0p    // ERROR "removed.* nil check"
 	_ = *intp       // ERROR "removed.* nil check"
 	_ = *arrayp     // ERROR "removed.* nil check"
+	_ = *array0p    // ERROR "removed.* nil check"
+	_ = *array0p    // ERROR "generated nil check"
+	_ = *intp       // ERROR "generated nil check"
+	_ = *arrayp     // ERROR "removed.* nil check"
 	_ = *structp    // ERROR "generated nil check"
 	_ = *emptyp     // ERROR "generated nil check"
-	_ = *arrayp     // ERROR "removed.* nil check"
+	_ = *arrayp     // ERROR "generated nil check"
 	_ = *bigarrayp  // ERROR "generated nil check" ARM removed nil check before indirect!!
 	_ = *bigstructp // ERROR "generated nil check"
 	_ = *empty1p    // ERROR "generated nil check"
@@ -122,16 +122,16 @@ func f3(x *[10000]int) {
 	// x wasn't going to change across the function call.
 	// But it's a little complex to do and in practice doesn't
 	// matter enough.
-	_ = x[9999] // ERROR "removed nil check"
+	_ = x[9999] // ERROR "generated nil check" // TODO: fix
 }
 
 func f3a() {
 	x := fx10k()
 	y := fx10k()
 	z := fx10k()
-	_ = &x[9] // ERROR "generated nil check"
-	y = z
 	_ = &x[9] // ERROR "removed.* nil check"
+	y = z
+	_ = &x[9] // ERROR "generated nil check"
 	x = y
 	_ = &x[9] // ERROR "generated nil check"
 }
@@ -139,11 +139,11 @@ func f3a() {
 func f3b() {
 	x := fx10k()
 	y := fx10k()
-	_ = &x[9] // ERROR "generated nil check"
+	_ = &x[9] // ERROR "removed.* nil check"
 	y = x
 	_ = &x[9] // ERROR "removed.* nil check"
 	x = y
-	_ = &x[9] // ERROR "removed.* nil check"
+	_ = &x[9] // ERROR "generated nil check"
 }
 
 func fx10() *[10]int
@@ -179,15 +179,15 @@ func f4(x *[10]int) {
 	_ = x[9] // ERROR "generated nil check"  // bug would like to remove before indirect
 
 	fx10()
-	_ = x[9] // ERROR "removed nil check"
+	_ = x[9] // ERROR "generated nil check"  // TODO: fix
 
 	x = fx10()
 	y := fx10()
-	_ = &x[9] // ERROR "generated nil check"
+	_ = &x[9] // ERROR "removed[a-z ]* nil check"
 	y = x
 	_ = &x[9] // ERROR "removed[a-z ]* nil check"
 	x = y
-	_ = &x[9] // ERROR "removed[a-z ]* nil check"
+	_ = &x[9] // ERROR "generated nil check"
 }
 
 func f5(p *float32, q *float64, r *float32, s *float64) float64 {