cmd/compile: remove incorrect arm,arm64 CMP->CMN transformations

These can go wrong when one of the operands is the minimum integer value.

Fixes #50854.

Change-Id: I238fe284f60c7ee5aeb9dc9a18e8b1578cdb77d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/381318
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/cmd/compile/internal/ssa/gen/ARM.rules

@@ -1270,8 +1270,8 @@
 (SRLconst (SLLconst x [c]) [d]) && buildcfg.GOARM==7 && uint64(d)>=uint64(c) && uint64(d)<=31 => (BFXU [(d-c)|(32-d)<<8] x)
 
 // comparison simplification
-((LT|LE|EQ|NE|GE|GT) (CMP x (RSBconst [0] y))) => ((LT|LE|EQ|NE|GE|GT) (CMN x y)) // sense of carry bit not preserved
+((EQ|NE) (CMP x (RSBconst [0] y))) => ((EQ|NE) (CMN x y)) // sense of carry bit not preserved; see also #50854
-((LT|LE|EQ|NE|GE|GT) (CMN x (RSBconst [0] y))) => ((LT|LE|EQ|NE|GE|GT) (CMP x y)) // sense of carry bit not preserved
+((EQ|NE) (CMN x (RSBconst [0] y))) => ((EQ|NE) (CMP x y)) // sense of carry bit not preserved; see also #50864
 (EQ (CMPconst [0] l:(SUB x y)) yes no) && l.Uses==1 => (EQ (CMP x y) yes no)
 (EQ (CMPconst [0] l:(MULS x y a)) yes no) && l.Uses==1 => (EQ (CMP a (MUL <x.Type> x y)) yes no)
 (EQ (CMPconst [0] l:(SUBconst [c] x)) yes no) && l.Uses==1 => (EQ (CMPconst [c] x) yes no)

src/cmd/compile/internal/ssa/gen/ARM64.rules

@@ -649,19 +649,13 @@
 (GT (CMPWconst [0] z:(ADD x y)) yes no) && z.Uses == 1 => (GTnoov (CMNW x y) yes no)
 (GE (CMPWconst [0] z:(ADD x y)) yes no) && z.Uses == 1 => (GEnoov (CMNW x y) yes no)
 
+// CMP(x,-y) -> CMN(x,y) is only valid for unordered comparison, if y can be -1<<63
 (EQ (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (EQ (CMN x y) yes no)
 (NE (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (NE (CMN x y) yes no)
-(LT (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (LT (CMN x y) yes no)
-(LE (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (LE (CMN x y) yes no)
-(GT (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (GT (CMN x y) yes no)
-(GE (CMP x z:(NEG y)) yes no) && z.Uses == 1 => (GE (CMN x y) yes no)
 
+// CMPW(x,-y) -> CMNW(x,y) is only valid for unordered comparison, if y can be -1<<31
 (EQ (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (EQ (CMNW x y) yes no)
 (NE (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (NE (CMNW x y) yes no)
-(LT (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (LT (CMNW x y) yes no)
-(LE (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (LE (CMNW x y) yes no)
-(GT (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (GT (CMNW x y) yes no)
-(GE (CMPW x z:(NEG y)) yes no) && z.Uses == 1 => (GE (CMNW x y) yes no)
 
 (EQ (CMPconst [0] x) yes no) => (Z x yes no)
 (NE (CMPconst [0] x) yes no) => (NZ x yes no)

src/cmd/compile/internal/ssa/gen/ARM64Ops.go

@@ -285,9 +285,9 @@ func init() {
 		{name: "CMPconst", argLength: 1, reg: gp1flags, asm: "CMP", aux: "Int64", typ: "Flags"},   // arg0 compare to auxInt
 		{name: "CMPW", argLength: 2, reg: gp2flags, asm: "CMPW", typ: "Flags"},                    // arg0 compare to arg1, 32 bit
 		{name: "CMPWconst", argLength: 1, reg: gp1flags, asm: "CMPW", aux: "Int32", typ: "Flags"}, // arg0 compare to auxInt, 32 bit
-		{name: "CMN", argLength: 2, reg: gp2flags, asm: "CMN", typ: "Flags", commutative: true},   // arg0 compare to -arg1
+		{name: "CMN", argLength: 2, reg: gp2flags, asm: "CMN", typ: "Flags", commutative: true},   // arg0 compare to -arg1, provided arg1 is not 1<<63
 		{name: "CMNconst", argLength: 1, reg: gp1flags, asm: "CMN", aux: "Int64", typ: "Flags"},   // arg0 compare to -auxInt
-		{name: "CMNW", argLength: 2, reg: gp2flags, asm: "CMNW", typ: "Flags", commutative: true}, // arg0 compare to -arg1, 32 bit
+		{name: "CMNW", argLength: 2, reg: gp2flags, asm: "CMNW", typ: "Flags", commutative: true}, // arg0 compare to -arg1, 32 bit, provided arg1 is not 1<<31
 		{name: "CMNWconst", argLength: 1, reg: gp1flags, asm: "CMNW", aux: "Int32", typ: "Flags"}, // arg0 compare to -auxInt, 32 bit
 		{name: "TST", argLength: 2, reg: gp2flags, asm: "TST", typ: "Flags", commutative: true},   // arg0 & arg1 compare to 0
 		{name: "TSTconst", argLength: 1, reg: gp1flags, asm: "TST", aux: "Int64", typ: "Flags"},   // arg0 & auxInt compare to 0

src/cmd/compile/internal/ssa/gen/ARMOps.go

@@ -331,7 +331,7 @@ func init() {
 		// comparisons
 		{name: "CMP", argLength: 2, reg: gp2flags, asm: "CMP", typ: "Flags"},                    // arg0 compare to arg1
 		{name: "CMPconst", argLength: 1, reg: gp1flags, asm: "CMP", aux: "Int32", typ: "Flags"}, // arg0 compare to auxInt
-		{name: "CMN", argLength: 2, reg: gp2flags, asm: "CMN", typ: "Flags", commutative: true}, // arg0 compare to -arg1
+		{name: "CMN", argLength: 2, reg: gp2flags, asm: "CMN", typ: "Flags", commutative: true}, // arg0 compare to -arg1, provided arg1 is not 1<<63
 		{name: "CMNconst", argLength: 1, reg: gp1flags, asm: "CMN", aux: "Int32", typ: "Flags"}, // arg0 compare to -auxInt
 		{name: "TST", argLength: 2, reg: gp2flags, asm: "TST", typ: "Flags", commutative: true}, // arg0 & arg1 compare to 0
 		{name: "TSTconst", argLength: 1, reg: gp1flags, asm: "TST", aux: "Int32", typ: "Flags"}, // arg0 & auxInt compare to 0

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

@@ -17153,42 +17153,6 @@ func rewriteBlockARM(b *Block) bool {
 			b.resetWithControl(BlockARMLE, cmp)
 			return true
 		}
-		// match: (GE (CMP x (RSBconst [0] y)))
-		// result: (GE (CMN x y))
-		for b.Controls[0].Op == OpARMCMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-				break
-			}
-			y := v_0_1.Args[0]
-			v0 := b.NewValue0(v_0.Pos, OpARMCMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARMGE, v0)
-			return true
-		}
-		// match: (GE (CMN x (RSBconst [0] y)))
-		// result: (GE (CMP x y))
-		for b.Controls[0].Op == OpARMCMN {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			v_0_0 := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			for _i0 := 0; _i0 <= 1; _i0, v_0_0, v_0_1 = _i0+1, v_0_1, v_0_0 {
-				x := v_0_0
-				if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-					continue
-				}
-				y := v_0_1.Args[0]
-				v0 := b.NewValue0(v_0.Pos, OpARMCMP, types.TypeFlags)
-				v0.AddArg2(x, y)
-				b.resetWithControl(BlockARMGE, v0)
-				return true
-			}
-			break
-		}
 		// match: (GE (CMPconst [0] l:(SUB x y)) yes no)
 		// cond: l.Uses==1
 		// result: (GEnoov (CMP x y) yes no)
@@ -18069,42 +18033,6 @@ func rewriteBlockARM(b *Block) bool {
 			b.resetWithControl(BlockARMLT, cmp)
 			return true
 		}
-		// match: (GT (CMP x (RSBconst [0] y)))
-		// result: (GT (CMN x y))
-		for b.Controls[0].Op == OpARMCMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-				break
-			}
-			y := v_0_1.Args[0]
-			v0 := b.NewValue0(v_0.Pos, OpARMCMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARMGT, v0)
-			return true
-		}
-		// match: (GT (CMN x (RSBconst [0] y)))
-		// result: (GT (CMP x y))
-		for b.Controls[0].Op == OpARMCMN {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			v_0_0 := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			for _i0 := 0; _i0 <= 1; _i0, v_0_0, v_0_1 = _i0+1, v_0_1, v_0_0 {
-				x := v_0_0
-				if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-					continue
-				}
-				y := v_0_1.Args[0]
-				v0 := b.NewValue0(v_0.Pos, OpARMCMP, types.TypeFlags)
-				v0.AddArg2(x, y)
-				b.resetWithControl(BlockARMGT, v0)
-				return true
-			}
-			break
-		}
 		// match: (GT (CMPconst [0] l:(SUB x y)) yes no)
 		// cond: l.Uses==1
 		// result: (GTnoov (CMP x y) yes no)
@@ -19076,42 +19004,6 @@ func rewriteBlockARM(b *Block) bool {
 			b.resetWithControl(BlockARMGE, cmp)
 			return true
 		}
-		// match: (LE (CMP x (RSBconst [0] y)))
-		// result: (LE (CMN x y))
-		for b.Controls[0].Op == OpARMCMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-				break
-			}
-			y := v_0_1.Args[0]
-			v0 := b.NewValue0(v_0.Pos, OpARMCMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARMLE, v0)
-			return true
-		}
-		// match: (LE (CMN x (RSBconst [0] y)))
-		// result: (LE (CMP x y))
-		for b.Controls[0].Op == OpARMCMN {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			v_0_0 := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			for _i0 := 0; _i0 <= 1; _i0, v_0_0, v_0_1 = _i0+1, v_0_1, v_0_0 {
-				x := v_0_0
-				if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-					continue
-				}
-				y := v_0_1.Args[0]
-				v0 := b.NewValue0(v_0.Pos, OpARMCMP, types.TypeFlags)
-				v0.AddArg2(x, y)
-				b.resetWithControl(BlockARMLE, v0)
-				return true
-			}
-			break
-		}
 		// match: (LE (CMPconst [0] l:(SUB x y)) yes no)
 		// cond: l.Uses==1
 		// result: (LEnoov (CMP x y) yes no)
@@ -19992,42 +19884,6 @@ func rewriteBlockARM(b *Block) bool {
 			b.resetWithControl(BlockARMGT, cmp)
 			return true
 		}
-		// match: (LT (CMP x (RSBconst [0] y)))
-		// result: (LT (CMN x y))
-		for b.Controls[0].Op == OpARMCMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-				break
-			}
-			y := v_0_1.Args[0]
-			v0 := b.NewValue0(v_0.Pos, OpARMCMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARMLT, v0)
-			return true
-		}
-		// match: (LT (CMN x (RSBconst [0] y)))
-		// result: (LT (CMP x y))
-		for b.Controls[0].Op == OpARMCMN {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			v_0_0 := v_0.Args[0]
-			v_0_1 := v_0.Args[1]
-			for _i0 := 0; _i0 <= 1; _i0, v_0_0, v_0_1 = _i0+1, v_0_1, v_0_0 {
-				x := v_0_0
-				if v_0_1.Op != OpARMRSBconst || auxIntToInt32(v_0_1.AuxInt) != 0 {
-					continue
-				}
-				y := v_0_1.Args[0]
-				v0 := b.NewValue0(v_0.Pos, OpARMCMP, types.TypeFlags)
-				v0.AddArg2(x, y)
-				b.resetWithControl(BlockARMLT, v0)
-				return true
-			}
-			break
-		}
 		// match: (LT (CMPconst [0] l:(SUB x y)) yes no)
 		// cond: l.Uses==1
 		// result: (LTnoov (CMP x y) yes no)

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

@@ -27983,46 +27983,6 @@ func rewriteBlockARM64(b *Block) bool {
 			}
 			break
 		}
-		// match: (GE (CMP x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (GE (CMN x y) yes no)
-		for b.Controls[0].Op == OpARM64CMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64GE, v0)
-			return true
-		}
-		// match: (GE (CMPW x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (GE (CMNW x y) yes no)
-		for b.Controls[0].Op == OpARM64CMPW {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMNW, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64GE, v0)
-			return true
-		}
 		// match: (GE (CMPconst [0] z:(MADD a x y)) yes no)
 		// cond: z.Uses==1
 		// result: (GEnoov (CMN a (MUL <x.Type> x y)) yes no)
@@ -28419,46 +28379,6 @@ func rewriteBlockARM64(b *Block) bool {
 			}
 			break
 		}
-		// match: (GT (CMP x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (GT (CMN x y) yes no)
-		for b.Controls[0].Op == OpARM64CMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64GT, v0)
-			return true
-		}
-		// match: (GT (CMPW x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (GT (CMNW x y) yes no)
-		for b.Controls[0].Op == OpARM64CMPW {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMNW, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64GT, v0)
-			return true
-		}
 		// match: (GT (CMPconst [0] z:(MADD a x y)) yes no)
 		// cond: z.Uses==1
 		// result: (GTnoov (CMN a (MUL <x.Type> x y)) yes no)
@@ -28951,46 +28871,6 @@ func rewriteBlockARM64(b *Block) bool {
 			}
 			break
 		}
-		// match: (LE (CMP x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (LE (CMN x y) yes no)
-		for b.Controls[0].Op == OpARM64CMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64LE, v0)
-			return true
-		}
-		// match: (LE (CMPW x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (LE (CMNW x y) yes no)
-		for b.Controls[0].Op == OpARM64CMPW {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMNW, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64LE, v0)
-			return true
-		}
 		// match: (LE (CMPconst [0] z:(MADD a x y)) yes no)
 		// cond: z.Uses==1
 		// result: (LEnoov (CMN a (MUL <x.Type> x y)) yes no)
@@ -29363,46 +29243,6 @@ func rewriteBlockARM64(b *Block) bool {
 			}
 			break
 		}
-		// match: (LT (CMP x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (LT (CMN x y) yes no)
-		for b.Controls[0].Op == OpARM64CMP {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMN, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64LT, v0)
-			return true
-		}
-		// match: (LT (CMPW x z:(NEG y)) yes no)
-		// cond: z.Uses == 1
-		// result: (LT (CMNW x y) yes no)
-		for b.Controls[0].Op == OpARM64CMPW {
-			v_0 := b.Controls[0]
-			_ = v_0.Args[1]
-			x := v_0.Args[0]
-			z := v_0.Args[1]
-			if z.Op != OpARM64NEG {
-				break
-			}
-			y := z.Args[0]
-			if !(z.Uses == 1) {
-				break
-			}
-			v0 := b.NewValue0(v_0.Pos, OpARM64CMNW, types.TypeFlags)
-			v0.AddArg2(x, y)
-			b.resetWithControl(BlockARM64LT, v0)
-			return true
-		}
 		// match: (LT (CMPconst [0] z:(MADD a x y)) yes no)
 		// cond: z.Uses==1
 		// result: (LTnoov (CMN a (MUL <x.Type> x y)) yes no)

test/fixedbugs/issue50854.go

@@ -0,0 +1,38 @@
+// run
+
+// Copyright 2022 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
+
+// This checks for incorrect application of CMP(-x,y) -> CMN(x,y) in arm and arm64
+
+//go:noinline
+func f(p int64, x, y int64) bool { return -x <= p && p <= y }
+
+//go:noinline
+func g(p int32, x, y int32) bool { return -x <= p && p <= y }
+
+// There are some more complicated patterns involving compares and shifts, try to trigger those.
+
+//go:noinline
+func h(p int64, x, y int64) bool { return -(x<<1) <= p && p <= y }
+
+//go:noinline
+func k(p int32, x, y int32) bool { return -(1<<x) <= p && p <= y }
+
+//go:noinline
+func check(b bool) {
+	if b {
+		return
+	}
+	panic("FAILURE")
+}
+
+func main() {
+	check(f(1, -1<<63, 1<<63-1))
+	check(g(1, -1<<31, 1<<31-1))
+	check(h(1, -1<<62, 1<<63-1))
+	check(k(1, 31, 1<<31-1))
+}