cmd/compile: fix sign-extension merging rules

If we have

  y = <int16> (MOVBQSX x)
  z = <int32> (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 <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules
index 724b921e8222e67d9db4e7df32d6fe726f78224a..bcc8378a4e259f3494bc660b61d80a34aab55d87 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/AMD64.rules
+++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules
@@ -2512,18 +2512,20 @@
 (BSFQ (ORQconst <t> [1<<16] (MOVWQZX x))) -> (BSFQ (ORQconst <t> [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 60d68db23d37545b990b6d0f44e1f6886f58f41e..c2f71a41cd19b20b2ef003c1a7fea5e0490a15f9 100644 (file)
--- 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 (file)
index 0000000..996bd63
--- /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))
+       }
+}