cmd/compile: fix MIPS SGTconst-with-shift rules

(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) -> (MOVWconst [1])

This rule is problematic. 1<<(32-uint32(d)) <= int32(c) meant to
say that it is true if c is greater than the largest possible
value of the right shift. But when d==1, 1<<(32-1) is negative
and results in the wrong comparison.

Rewrite the rules in a more direct way.

Fixes #29402.

Change-Id: I5940fc9538d9bc3a4bcae8aa34672867540dc60e
Reviewed-on: https://go-review.googlesource.com/c/155798
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/ssa/gen/MIPS.rules b/src/cmd/compile/internal/ssa/gen/MIPS.rules
index 098e19c8a82642d2fab9dae369949c96d49d4553..db9c5bc63878b1e786854fd3161496cceb951910 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/MIPS.rules
+++ b/src/cmd/compile/internal/ssa/gen/MIPS.rules
@@ -670,8 +670,8 @@
 (SGTUconst [c] (MOVHUreg _)) && 0xffff < uint32(c) -> (MOVWconst [1])
 (SGTconst [c] (ANDconst [m] _)) && 0 <= int32(m) && int32(m) < int32(c) -> (MOVWconst [1])
 (SGTUconst [c] (ANDconst [m] _)) && uint32(m) < uint32(c) -> (MOVWconst [1])
-(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) -> (MOVWconst [1])
-(SGTUconst [c] (SRLconst _ [d])) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c) -> (MOVWconst [1])
+(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) -> (MOVWconst [1])
+(SGTUconst [c] (SRLconst _ [d])) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c) -> (MOVWconst [1])
 
 // absorb constants into branches
 (EQ  (MOVWconst [0]) yes no) -> (First nil yes no)

diff --git a/src/cmd/compile/internal/ssa/gen/MIPS64.rules b/src/cmd/compile/internal/ssa/gen/MIPS64.rules
index 70f4f0d616f75ae701bb18a9afe41e64bfc5bdf5..9c16c3543847feed48a33aa8e008137595bc5234 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/MIPS64.rules
+++ b/src/cmd/compile/internal/ssa/gen/MIPS64.rules
@@ -667,8 +667,8 @@
 (SGTconst [c] (MOVWUreg _)) && c < 0 -> (MOVVconst [0])
 (SGTconst [c] (ANDconst [m] _)) && 0 <= m && m < c -> (MOVVconst [1])
 (SGTUconst [c] (ANDconst [m] _)) && uint64(m) < uint64(c) -> (MOVVconst [1])
-(SGTconst [c] (SRLVconst _ [d])) && 0 <= c && 0 < d && d <= 63 && 1<<uint64(64-d) <= c -> (MOVVconst [1])
-(SGTUconst [c] (SRLVconst _ [d])) && 0 < d && d <= 63 && 1<<uint64(64-d) <= uint64(c) -> (MOVVconst [1])
+(SGTconst [c] (SRLVconst _ [d])) && 0 <= c && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c) -> (MOVVconst [1])
+(SGTUconst [c] (SRLVconst _ [d])) && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c) -> (MOVVconst [1])
 
 // absorb constants into branches
 (EQ  (MOVVconst [0]) yes no) -> (First nil yes no)

diff --git a/src/cmd/compile/internal/ssa/rewriteMIPS.go b/src/cmd/compile/internal/ssa/rewriteMIPS.go
index e51398185259875ae12293690aba4ac5d25cb920..951c5a5ef87f62f577ee876412a8d920e2063672 100644 (file)
--- a/src/cmd/compile/internal/ssa/rewriteMIPS.go
+++ b/src/cmd/compile/internal/ssa/rewriteMIPS.go
@@ -5625,7 +5625,7 @@ func rewriteValueMIPS_OpMIPSSGTUconst_0(v *Value) bool {
                return true
        }
        // match: (SGTUconst [c] (SRLconst _ [d]))
-       // cond: uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c)
+       // cond: uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)
        // result: (MOVWconst [1])
        for {
                c := v.AuxInt
@@ -5634,7 +5634,7 @@ func rewriteValueMIPS_OpMIPSSGTUconst_0(v *Value) bool {
                        break
                }
                d := v_0.AuxInt
-               if !(uint32(d) <= 31 && 1<<(32-uint32(d)) <= uint32(c)) {
+               if !(uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)) {
                        break
                }
                v.reset(OpMIPSMOVWconst)
@@ -5862,7 +5862,7 @@ func rewriteValueMIPS_OpMIPSSGTconst_10(v *Value) bool {
                return true
        }
        // match: (SGTconst [c] (SRLconst _ [d]))
-       // cond: 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c)
+       // cond: 0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)
        // result: (MOVWconst [1])
        for {
                c := v.AuxInt
@@ -5871,7 +5871,7 @@ func rewriteValueMIPS_OpMIPSSGTconst_10(v *Value) bool {
                        break
                }
                d := v_0.AuxInt
-               if !(0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c)) {
+               if !(0 <= int32(c) && uint32(d) <= 31 && 0xffffffff>>uint32(d) < uint32(c)) {
                        break
                }
                v.reset(OpMIPSMOVWconst)

diff --git a/src/cmd/compile/internal/ssa/rewriteMIPS64.go b/src/cmd/compile/internal/ssa/rewriteMIPS64.go
index 04df5b8603d6b7c4e029343067119d694184794a..9e12780664d60a624e183687ba0e13753b6ef01c 100644 (file)
--- a/src/cmd/compile/internal/ssa/rewriteMIPS64.go
+++ b/src/cmd/compile/internal/ssa/rewriteMIPS64.go
@@ -6005,7 +6005,7 @@ func rewriteValueMIPS64_OpMIPS64SGTUconst_0(v *Value) bool {
                return true
        }
        // match: (SGTUconst [c] (SRLVconst _ [d]))
-       // cond: 0 < d && d <= 63 && 1<<uint64(64-d) <= uint64(c)
+       // cond: 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c)
        // result: (MOVVconst [1])
        for {
                c := v.AuxInt
@@ -6014,7 +6014,7 @@ func rewriteValueMIPS64_OpMIPS64SGTUconst_0(v *Value) bool {
                        break
                }
                d := v_0.AuxInt
-               if !(0 < d && d <= 63 && 1<<uint64(64-d) <= uint64(c)) {
+               if !(0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c)) {
                        break
                }
                v.reset(OpMIPS64MOVVconst)
@@ -6223,7 +6223,7 @@ func rewriteValueMIPS64_OpMIPS64SGTconst_10(v *Value) bool {
                return true
        }
        // match: (SGTconst [c] (SRLVconst _ [d]))
-       // cond: 0 <= c && 0 < d && d <= 63 && 1<<uint64(64-d) <= c
+       // cond: 0 <= c && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c)
        // result: (MOVVconst [1])
        for {
                c := v.AuxInt
@@ -6232,7 +6232,7 @@ func rewriteValueMIPS64_OpMIPS64SGTconst_10(v *Value) bool {
                        break
                }
                d := v_0.AuxInt
-               if !(0 <= c && 0 < d && d <= 63 && 1<<uint64(64-d) <= c) {
+               if !(0 <= c && 0 < d && d <= 63 && 0xffffffffffffffff>>uint64(d) < uint64(c)) {
                        break
                }
                v.reset(OpMIPS64MOVVconst)

diff --git a/test/fixedbugs/issue29402.go b/test/fixedbugs/issue29402.go
new file mode 100644 (file)
index 0000000..8a1f959
--- /dev/null
+++ b/test/fixedbugs/issue29402.go
@@ -0,0 +1,23 @@
+// 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.
+
+// Issue 29402: wrong optimization of comparison of
+// constant and shift on MIPS.
+
+package main
+
+//go:noinline
+func F(s []int) bool {
+       half := len(s) / 2
+       return half >= 0
+}
+
+func main() {
+       b := F([]int{1, 2, 3, 4})
+       if !b {
+               panic("FAIL")
+       }
+}