]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: ensure that loop condition is detected correctly
authorKeith Randall <khr@google.com>
Mon, 2 Jul 2018 22:21:35 +0000 (15:21 -0700)
committerKeith Randall <khr@golang.org>
Mon, 9 Jul 2018 18:23:39 +0000 (18:23 +0000)
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
test/fixedbugs/issue26116.go [new file with mode: 0644]
test/loopbce.go
test/prove.go

index 2ab05711ad3d71c3e60194500f795e97e593df37..8ab1a0c695e70319937c4ac9518f01f35914398a 100644 (file)
@@ -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)
diff --git a/test/fixedbugs/issue26116.go b/test/fixedbugs/issue26116.go
new file mode 100644 (file)
index 0000000..8a63481
--- /dev/null
@@ -0,0 +1,18 @@
+// 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")
+       }
+}
index c93bfc8f000fb69c5bb260f2f3f32cc4b5dcce0a..b4bf797497f0c0949fba7544bc334ae6c0ad10b1 100644 (file)
@@ -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) {
 }
index 9de7d1b3fc8f93cfa553e013eebb7a32fe09ef59..45cee9e8b58037270f0f7e23aa23c4d53d5a3425 100644 (file)
@@ -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--