From 4d284ea05230c37a653053c163f0eb8b1f9b6138 Mon Sep 17 00:00:00 2001 From: David Chase Date: Thu, 27 Jan 2022 11:26:59 -0500 Subject: [release-branch.go1.16] cmd/compile: remove incorrect arm,arm64 CMP->CMN transformations These can go wrong when one of the operands is the minimum integer value. Fixes #50866. Change-Id: I238fe284f60c7ee5aeb9dc9a18e8b1578cdb77d0 Reviewed-on: https://go-review.googlesource.com/c/go/+/381318 Reviewed-by: Keith Randall Reviewed-by: Cherry Mui Trust: David Chase Run-TryBot: David Chase TryBot-Result: Gopher Robot (cherry picked from commit b7b44b3173f151a2313da7072afd25de80511605) Reviewed-on: https://go-review.googlesource.com/c/go/+/381475 Trust: Michael Knyszek Run-TryBot: Cherry Mui --- src/cmd/compile/internal/ssa/gen/ARM.rules | 4 +- src/cmd/compile/internal/ssa/gen/ARM64.rules | 10 +- src/cmd/compile/internal/ssa/gen/ARM64Ops.go | 4 +- src/cmd/compile/internal/ssa/gen/ARMOps.go | 2 +- src/cmd/compile/internal/ssa/rewriteARM.go | 144 ------------------------ src/cmd/compile/internal/ssa/rewriteARM64.go | 160 --------------------------- test/fixedbugs/issue50854.go | 38 +++++++ 7 files changed, 45 insertions(+), 317 deletions(-) create mode 100644 test/fixedbugs/issue50854.go diff --git a/src/cmd/compile/internal/ssa/gen/ARM.rules b/src/cmd/compile/internal/ssa/gen/ARM.rules index 69989b0c45..2a30f9c342 100644 --- a/src/cmd/compile/internal/ssa/gen/ARM.rules +++ b/src/cmd/compile/internal/ssa/gen/ARM.rules @@ -1263,8 +1263,8 @@ (SRLconst (SLLconst x [c]) [d]) && objabi.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 -((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) (CMP x (RSBconst [0] y))) => ((EQ|NE) (CMN x y)) // sense of carry bit not preserved; see also #50854 +((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 y)) yes no) (EQ (CMPconst [0] l:(SUBconst [c] x)) yes no) && l.Uses==1 => (EQ (CMPconst [c] x) yes no) diff --git a/src/cmd/compile/internal/ssa/gen/ARM64.rules b/src/cmd/compile/internal/ssa/gen/ARM64.rules index 80b4005df1..3c66494ab5 100644 --- a/src/cmd/compile/internal/ssa/gen/ARM64.rules +++ b/src/cmd/compile/internal/ssa/gen/ARM64.rules @@ -643,19 +643,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) diff --git a/src/cmd/compile/internal/ssa/gen/ARM64Ops.go b/src/cmd/compile/internal/ssa/gen/ARM64Ops.go index 4d1d14e18b..fc3dcce629 100644 --- a/src/cmd/compile/internal/ssa/gen/ARM64Ops.go +++ b/src/cmd/compile/internal/ssa/gen/ARM64Ops.go @@ -281,9 +281,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 diff --git a/src/cmd/compile/internal/ssa/gen/ARMOps.go b/src/cmd/compile/internal/ssa/gen/ARMOps.go index 1a7eefa50a..7516c4ffe9 100644 --- a/src/cmd/compile/internal/ssa/gen/ARMOps.go +++ b/src/cmd/compile/internal/ssa/gen/ARMOps.go @@ -329,7 +329,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 diff --git a/src/cmd/compile/internal/ssa/rewriteARM.go b/src/cmd/compile/internal/ssa/rewriteARM.go index 1f25005eb7..8da5b40cf5 100644 --- a/src/cmd/compile/internal/ssa/rewriteARM.go +++ b/src/cmd/compile/internal/ssa/rewriteARM.go @@ -17123,42 +17123,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) @@ -18039,42 +18003,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) @@ -19046,42 +18974,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) @@ -19962,42 +19854,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) diff --git a/src/cmd/compile/internal/ssa/rewriteARM64.go b/src/cmd/compile/internal/ssa/rewriteARM64.go index e61d89992d..5afccf163d 100644 --- a/src/cmd/compile/internal/ssa/rewriteARM64.go +++ b/src/cmd/compile/internal/ssa/rewriteARM64.go @@ -26214,46 +26214,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 y)) yes no) @@ -26650,46 +26610,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 y)) yes no) @@ -27182,46 +27102,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 y)) yes no) @@ -27594,46 +27474,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 y)) yes no) diff --git a/test/fixedbugs/issue50854.go b/test/fixedbugs/issue50854.go new file mode 100644 index 0000000000..a5be9195f1 --- /dev/null +++ b/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<