cmd/compile: ensure that loop condition is detected correctly

We need to make sure that the terminating comparison has the right
sense given the increment direction. If the increment is positive,
the terminating comparsion must be < or <=. If the increment is
negative, the terminating comparison must be > or >=.

Do a few cleanups,  like constant-folding entry==0, adding comments,
removing unused "exported" fields.

Fixes #26116

Change-Id: I14230ee8126054b750e2a1f2b18eb8f09873dbd5
Reviewed-on: https://go-review.googlesource.com/121940
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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

@@ -15,15 +15,15 @@ const (
 
 type indVar struct {
 	ind   *Value // induction variable
-	inc   *Value // increment, a constant
-	nxt   *Value // ind+inc variable
 	min   *Value // minimum value, inclusive/exclusive depends on flags
 	max   *Value // maximum value, inclusive/exclusive depends on flags
 	entry *Block // entry block in the loop.
 	flags indVarFlags
-	// Invariants: for all blocks dominated by entry:
-	//	min <= ind < max
-	//	min <= nxt <= max
+	// Invariant: for all blocks strictly dominated by entry:
+	//	min <= ind <  max    [if flags == 0]
+	//	min <  ind <  max    [if flags == indVarMinExc]
+	//	min <= ind <= max    [if flags == indVarMaxInc]
+	//	min <  ind <= max    [if flags == indVarMinExc|indVarMaxInc]
 }
 
 // findIndVar finds induction variables in a function.
@@ -49,7 +49,6 @@ func findIndVar(f *Func) []indVar {
 	var iv []indVar
 	sdom := f.sdom()
 
-nextb:
 	for _, b := range f.Blocks {
 		if b.Kind != BlockIf || len(b.Preds) != 2 {
 			continue
@@ -57,7 +56,6 @@ nextb:
 
 		var flags indVarFlags
 		var ind, max *Value // induction, and maximum
-		entry := -1         // which successor of b enters the loop
 
 		// Check thet the control if it either ind </<= max or max >/>= ind.
 		// TODO: Handle 32-bit comparisons.
@@ -66,21 +64,21 @@ nextb:
 			flags |= indVarMaxInc
 			fallthrough
 		case OpLess64:
-			entry = 0
 			ind, max = b.Control.Args[0], b.Control.Args[1]
 		case OpGeq64:
 			flags |= indVarMaxInc
 			fallthrough
 		case OpGreater64:
-			entry = 0
 			ind, max = b.Control.Args[1], b.Control.Args[0]
 		default:
-			continue nextb
+			continue
 		}
 
 		// See if the arguments are reversed (i < len() <=> len() > i)
+		less := true
 		if max.Op == OpPhi {
 			ind, max = max, ind
+			less = false
 		}
 
 		// Check that the induction variable is a phi that depends on itself.
@@ -108,22 +106,35 @@ nextb:
 			panic("unreachable") // one of the cases must be true from the above.
 		}
 
-		// Expect the increment to be a constant.
+		// Expect the increment to be a nonzero constant.
 		if inc.Op != OpConst64 {
 			continue
 		}
+		step := inc.AuxInt
+		if step == 0 {
+			continue
+		}
+
+		// Increment sign must match comparison direction.
+		// When incrementing, the termination comparison must be ind </<= max.
+		// When decrementing, the termination comparison must be ind >/>= max.
+		// See issue 26116.
+		if step > 0 && !less {
+			continue
+		}
+		if step < 0 && less {
+			continue
+		}
 
 		// If the increment is negative, swap min/max and their flags
-		if inc.AuxInt <= 0 {
+		if step < 0 {
 			min, max = max, min
 			oldf := flags
-			flags = 0
+			flags = indVarMaxInc
 			if oldf&indVarMaxInc == 0 {
 				flags |= indVarMinExc
 			}
-			if oldf&indVarMinExc == 0 {
-				flags |= indVarMaxInc
-			}
+			step = -step
 		}
 
 		// Up to now we extracted the induction variable (ind),
@@ -140,26 +151,26 @@ nextb:
 		// as an induction variable.
 
 		// First condition: loop entry has a single predecessor, which
-		// is the header block.  This implies that b.Succs[entry] is
+		// is the header block.  This implies that b.Succs[0] is
 		// reached iff ind < max.
-		if len(b.Succs[entry].b.Preds) != 1 {
-			// b.Succs[1-entry] must exit the loop.
+		if len(b.Succs[0].b.Preds) != 1 {
+			// b.Succs[1] must exit the loop.
 			continue
 		}
 
-		// Second condition: b.Succs[entry] dominates nxt so that
+		// Second condition: b.Succs[0] dominates nxt so that
 		// nxt is computed when inc < max, meaning nxt <= max.
-		if !sdom.isAncestorEq(b.Succs[entry].b, nxt.Block) {
+		if !sdom.isAncestorEq(b.Succs[0].b, nxt.Block) {
 			// inc+ind can only be reached through the branch that enters the loop.
 			continue
 		}
 
 		// We can only guarantee that the loops runs within limits of induction variable
 		// if the increment is ±1 or when the limits are constants.
-		if inc.AuxInt != 1 && inc.AuxInt != -1 {
+		if step != 1 {
 			ok := false
-			if min.Op == OpConst64 && max.Op == OpConst64 && inc.AuxInt != 0 {
-				if max.AuxInt > min.AuxInt && max.AuxInt%inc.AuxInt == min.AuxInt%inc.AuxInt { // handle overflow
+			if min.Op == OpConst64 && max.Op == OpConst64 {
+				if max.AuxInt > min.AuxInt && max.AuxInt%step == min.AuxInt%step { // handle overflow
 					ok = true
 				}
 			}
@@ -169,16 +180,14 @@ nextb:
 		}
 
 		if f.pass.debug >= 1 {
-			printIndVar(b, ind, min, max, inc.AuxInt, flags)
+			printIndVar(b, ind, min, max, step, flags)
 		}
 
 		iv = append(iv, indVar{
 			ind:   ind,
-			inc:   inc,
-			nxt:   nxt,
 			min:   min,
 			max:   max,
-			entry: b.Succs[entry].b,
+			entry: b.Succs[0].b,
 			flags: flags,
 		})
 		b.Logf("found induction variable %v (inc = %v, min = %v, max = %v)\n", ind, inc, min, max)

test/fixedbugs/issue26116.go

+// run
+
+// Copyright 2018 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
+
+func main() {
+	s := []int{0, 1, 2}
+	i := 1
+	for i > 0 && s[i] != 2 {
+		i++
+	}
+	if i != 2 {
+		panic("loop didn't run")
+	}
+}

test/loopbce.go

@@ -86,7 +86,7 @@ func g0b(a string) int {
 
 func g0c(a string) int {
 	x := 0
-	for i := len(a); i > 0; i-- { // ERROR "Induction variable: limits \(0,\?\], increment -1$"
+	for i := len(a); i > 0; i-- { // ERROR "Induction variable: limits \(0,\?\], increment 1$"
 		x += int(a[i-1]) // ERROR "Proved IsInBounds$"
 	}
 	return x
@@ -94,7 +94,7 @@ func g0c(a string) int {
 
 func g0d(a string) int {
 	x := 0
-	for i := len(a); 0 < i; i-- { // ERROR "Induction variable: limits \(0,\?\], increment -1$"
+	for i := len(a); 0 < i; i-- { // ERROR "Induction variable: limits \(0,\?\], increment 1$"
 		x += int(a[i-1]) // ERROR "Proved IsInBounds$"
 	}
 	return x
@@ -102,7 +102,7 @@ func g0d(a string) int {
 
 func g0e(a string) int {
 	x := 0
-	for i := len(a) - 1; i >= 0; i-- { // ERROR "Induction variable: limits \[0,\?\], increment -1$"
+	for i := len(a) - 1; i >= 0; i-- { // ERROR "Induction variable: limits \[0,\?\], increment 1$"
 		x += int(a[i]) // ERROR "Proved IsInBounds$"
 	}
 	return x
@@ -110,7 +110,7 @@ func g0e(a string) int {
 
 func g0f(a string) int {
 	x := 0
-	for i := len(a) - 1; 0 <= i; i-- { // ERROR "Induction variable: limits \[0,\?\], increment -1$"
+	for i := len(a) - 1; 0 <= i; i-- { // ERROR "Induction variable: limits \[0,\?\], increment 1$"
 		x += int(a[i]) // ERROR "Proved IsInBounds$"
 	}
 	return x
@@ -223,7 +223,7 @@ func k3(a [100]int) [100]int {
 }
 
 func k3neg(a [100]int) [100]int {
-	for i := 89; i > -11; i-- { // ERROR "Induction variable: limits \(-11,89\], increment -1$"
+	for i := 89; i > -11; i-- { // ERROR "Induction variable: limits \(-11,89\], increment 1$"
 		a[i+9] = i
 		a[i+10] = i // ERROR "Proved IsInBounds$"
 		a[i+11] = i
@@ -232,7 +232,7 @@ func k3neg(a [100]int) [100]int {
 }
 
 func k3neg2(a [100]int) [100]int {
-	for i := 89; i >= -10; i-- { // ERROR "Induction variable: limits \[-10,89\], increment -1$"
+	for i := 89; i >= -10; i-- { // ERROR "Induction variable: limits \[-10,89\], increment 1$"
 		a[i+9] = i
 		a[i+10] = i // ERROR "Proved IsInBounds$"
 		a[i+11] = i
@@ -302,6 +302,16 @@ func nobce3(a [100]int64) [100]int64 {
 	return a
 }
 
+func issue26116a(a []int) {
+	// There is no induction variable here. The comparison is in the wrong direction.
+	for i := 3; i > 6; i++ {
+		a[i] = 0
+	}
+	for i := 7; i < 3; i-- {
+		a[i] = 1
+	}
+}
+
 //go:noinline
 func useString(a string) {
 }

test/prove.go

@@ -622,7 +622,7 @@ func natcmp(x, y []uint) (r int) {
 	}
 
 	i := m - 1
-	for i > 0 && // ERROR "Induction variable: limits \(0,\?\], increment -1"
+	for i > 0 && // ERROR "Induction variable: limits \(0,\?\], increment 1"
 		x[i] == // ERROR "Proved IsInBounds$"
 			y[i] { // ERROR "Proved IsInBounds$"
 		i--