From 3f04db41a8dbf6f64304f3e1d34b4c7775fbe55e Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Thu, 21 Sep 2017 12:52:38 -0700 Subject: [PATCH] cmd/compile: fix sign-extension merging rules If we have y = (MOVBQSX x) z = (MOVWQSX y) We used to use this rewrite rule: (MOVWQSX x:(MOVBQSX _)) -> x But that resulted in replacing z with a value whose type is only int16. Then if z is spilled and restored, it gets zero extended instead of sign extended. Instead use the rule (MOVWQSX (MOVBQSX x)) -> (MOVBQSX x) The result is has the correct type, so it can be spilled and restored correctly. It might mean that a few more extension ops might not be eliminated, but that's the price for correctness. Fixes #21963 Change-Id: I6ec82c3d2dbe43cc1fee6fb2bd6b3a72fca3af00 Reviewed-on: https://go-review.googlesource.com/65290 Reviewed-by: Cherry Zhang Run-TryBot: Cherry Zhang TryBot-Result: Gobot Gobot --- src/cmd/compile/internal/ssa/gen/AMD64.rules | 26 ++-- src/cmd/compile/internal/ssa/rewriteAMD64.go | 144 +++++++++---------- test/fixedbugs/issue21963.go | 27 ++++ 3 files changed, 113 insertions(+), 84 deletions(-) create mode 100644 test/fixedbugs/issue21963.go diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules index 724b921e822..bcc8378a4e2 100644 --- a/src/cmd/compile/internal/ssa/gen/AMD64.rules +++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules @@ -2512,18 +2512,20 @@ (BSFQ (ORQconst [1<<16] (MOVWQZX x))) -> (BSFQ (ORQconst [1<<16] x)) // Redundant sign/zero extensions -(MOVLQSX x:(MOVLQSX _)) -> x -(MOVLQSX x:(MOVWQSX _)) -> x -(MOVLQSX x:(MOVBQSX _)) -> x -(MOVWQSX x:(MOVWQSX _)) -> x -(MOVWQSX x:(MOVBQSX _)) -> x -(MOVBQSX x:(MOVBQSX _)) -> x -(MOVLQZX x:(MOVLQZX _)) -> x -(MOVLQZX x:(MOVWQZX _)) -> x -(MOVLQZX x:(MOVBQZX _)) -> x -(MOVWQZX x:(MOVWQZX _)) -> x -(MOVWQZX x:(MOVBQZX _)) -> x -(MOVBQZX x:(MOVBQZX _)) -> x +// Note: see issue 21963. We have to make sure we use the right type on +// the resulting extension (the outer type, not the inner type). +(MOVLQSX (MOVLQSX x)) -> (MOVLQSX x) +(MOVLQSX (MOVWQSX x)) -> (MOVWQSX x) +(MOVLQSX (MOVBQSX x)) -> (MOVBQSX x) +(MOVWQSX (MOVWQSX x)) -> (MOVWQSX x) +(MOVWQSX (MOVBQSX x)) -> (MOVBQSX x) +(MOVBQSX (MOVBQSX x)) -> (MOVBQSX x) +(MOVLQZX (MOVLQZX x)) -> (MOVLQZX x) +(MOVLQZX (MOVWQZX x)) -> (MOVWQZX x) +(MOVLQZX (MOVBQZX x)) -> (MOVBQZX x) +(MOVWQZX (MOVWQZX x)) -> (MOVWQZX x) +(MOVWQZX (MOVBQZX x)) -> (MOVBQZX x) +(MOVBQZX (MOVBQZX x)) -> (MOVBQZX x) (MOVQstore [off] {sym} ptr a:(ADDQconst [c] l:(MOVQload [off] {sym} ptr2 mem)) mem) && isSamePtr(ptr, ptr2) && a.Uses == 1 && l.Uses == 1 && validValAndOff(c,off) -> diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go index 60d68db23d3..c2f71a41cd1 100644 --- a/src/cmd/compile/internal/ssa/rewriteAMD64.go +++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go @@ -4671,16 +4671,16 @@ func rewriteValueAMD64_OpAMD64MOVBQSX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVBQSX x:(MOVBQSX _)) + // match: (MOVBQSX (MOVBQSX x)) // cond: - // result: x + // result: (MOVBQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQSX) v.AddArg(x) return true } @@ -4888,16 +4888,16 @@ func rewriteValueAMD64_OpAMD64MOVBQZX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVBQZX x:(MOVBQZX _)) + // match: (MOVBQZX (MOVBQZX x)) // cond: - // result: x + // result: (MOVBQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQZX) v.AddArg(x) return true } @@ -6815,42 +6815,42 @@ func rewriteValueAMD64_OpAMD64MOVLQSX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVLQSX x:(MOVLQSX _)) + // match: (MOVLQSX (MOVLQSX x)) // cond: - // result: x + // result: (MOVLQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVLQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVLQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVLQSX) v.AddArg(x) return true } - // match: (MOVLQSX x:(MOVWQSX _)) + // match: (MOVLQSX (MOVWQSX x)) // cond: - // result: x + // result: (MOVWQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVWQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVWQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVWQSX) v.AddArg(x) return true } - // match: (MOVLQSX x:(MOVBQSX _)) + // match: (MOVLQSX (MOVBQSX x)) // cond: - // result: x + // result: (MOVBQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQSX) v.AddArg(x) return true } @@ -7047,42 +7047,42 @@ func rewriteValueAMD64_OpAMD64MOVLQZX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVLQZX x:(MOVLQZX _)) + // match: (MOVLQZX (MOVLQZX x)) // cond: - // result: x + // result: (MOVLQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVLQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVLQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVLQZX) v.AddArg(x) return true } - // match: (MOVLQZX x:(MOVWQZX _)) + // match: (MOVLQZX (MOVWQZX x)) // cond: - // result: x + // result: (MOVWQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVWQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVWQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVWQZX) v.AddArg(x) return true } - // match: (MOVLQZX x:(MOVBQZX _)) + // match: (MOVLQZX (MOVBQZX x)) // cond: - // result: x + // result: (MOVBQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQZX) v.AddArg(x) return true } @@ -12005,29 +12005,29 @@ func rewriteValueAMD64_OpAMD64MOVWQSX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVWQSX x:(MOVWQSX _)) + // match: (MOVWQSX (MOVWQSX x)) // cond: - // result: x + // result: (MOVWQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVWQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVWQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVWQSX) v.AddArg(x) return true } - // match: (MOVWQSX x:(MOVBQSX _)) + // match: (MOVWQSX (MOVBQSX x)) // cond: - // result: x + // result: (MOVBQSX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQSX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQSX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQSX) v.AddArg(x) return true } @@ -12237,29 +12237,29 @@ func rewriteValueAMD64_OpAMD64MOVWQZX_0(v *Value) bool { v.AddArg(x) return true } - // match: (MOVWQZX x:(MOVWQZX _)) + // match: (MOVWQZX (MOVWQZX x)) // cond: - // result: x + // result: (MOVWQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVWQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVWQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVWQZX) v.AddArg(x) return true } - // match: (MOVWQZX x:(MOVBQZX _)) + // match: (MOVWQZX (MOVBQZX x)) // cond: - // result: x + // result: (MOVBQZX x) for { - x := v.Args[0] - if x.Op != OpAMD64MOVBQZX { + v_0 := v.Args[0] + if v_0.Op != OpAMD64MOVBQZX { break } - v.reset(OpCopy) - v.Type = x.Type + x := v_0.Args[0] + v.reset(OpAMD64MOVBQZX) v.AddArg(x) return true } diff --git a/test/fixedbugs/issue21963.go b/test/fixedbugs/issue21963.go new file mode 100644 index 00000000000..996bd63d09d --- /dev/null +++ b/test/fixedbugs/issue21963.go @@ -0,0 +1,27 @@ +// run + +// Copyright 2017 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 + +import ( + "fmt" + "runtime" +) + +//go:noinline +func f(x []int32, y *int8) int32 { + c := int32(int16(*y)) + runtime.GC() + return x[0] * c +} + +func main() { + var x = [1]int32{5} + var y int8 = -1 + if got, want := f(x[:], &y), int32(-5); got != want { + panic(fmt.Sprintf("wanted %d, got %d", want, got)) + } +}