[release-branch.go1.9] cmd/compile: fix constant folding of right shifts

The sub-word shifts need to sign-extend before shifting, to avoid
bringing in data from higher in the argument.

Fixes #23812

Change-Id: I0a95a0b49c48f3b40b85765bb4a9bb492be0cd73
Reviewed-on: https://go-review.googlesource.com/93716
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/102775
Run-TryBot: Andrew Bonventre <andybons@golang.org>

diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules
index ff38be550e6baec0a1567077dfa919a9dde43472..89d082ac7f937e851fdaed25c403095551948195 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/AMD64.rules
+++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules
@@ -1499,9 +1499,9 @@
 (SUBQconst (MOVQconst [d]) [c]) -> (MOVQconst [d-c])
 (SUBQconst (SUBQconst x [d]) [c]) && is32Bit(-c-d) -> (ADDQconst [-c-d] x)
 (SARQconst [c] (MOVQconst [d])) -> (MOVQconst [d>>uint64(c)])
-(SARLconst [c] (MOVQconst [d])) -> (MOVQconst [d>>uint64(c)])
-(SARWconst [c] (MOVQconst [d])) -> (MOVQconst [d>>uint64(c)])
-(SARBconst [c] (MOVQconst [d])) -> (MOVQconst [d>>uint64(c)])
+(SARLconst [c] (MOVQconst [d])) -> (MOVQconst [int64(int32(d))>>uint64(c)])
+(SARWconst [c] (MOVQconst [d])) -> (MOVQconst [int64(int16(d))>>uint64(c)])
+(SARBconst [c] (MOVQconst [d])) -> (MOVQconst [int64(int8(d))>>uint64(c)])
 (NEGQ (MOVQconst [c])) -> (MOVQconst [-c])
 (NEGL (MOVLconst [c])) -> (MOVLconst [int64(int32(-c))])
 (MULQconst [c] (MOVQconst [d])) -> (MOVQconst [c*d])

diff --git a/src/cmd/compile/internal/ssa/gen/AMD64Ops.go b/src/cmd/compile/internal/ssa/gen/AMD64Ops.go
index c984cbfb1272975cf36332614c6d1c41a62cd024..12757a7d5a2ef51e2a11d342809a3b5cce0a2cea 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/AMD64Ops.go
+++ b/src/cmd/compile/internal/ssa/gen/AMD64Ops.go
@@ -268,22 +268,22 @@ func init() {
                // Note: x86 is weird, the 16 and 8 byte shifts still use all 5 bits of shift amount!
 
                {name: "SHRQ", argLength: 2, reg: gp21shift, asm: "SHRQ", resultInArg0: true, clobberFlags: true},              // unsigned arg0 >> arg1, shift amount is mod 64
-               {name: "SHRL", argLength: 2, reg: gp21shift, asm: "SHRL", resultInArg0: true, clobberFlags: true},              // unsigned arg0 >> arg1, shift amount is mod 32
-               {name: "SHRW", argLength: 2, reg: gp21shift, asm: "SHRW", resultInArg0: true, clobberFlags: true},              // unsigned arg0 >> arg1, shift amount is mod 32
-               {name: "SHRB", argLength: 2, reg: gp21shift, asm: "SHRB", resultInArg0: true, clobberFlags: true},              // unsigned arg0 >> arg1, shift amount is mod 32
+               {name: "SHRL", argLength: 2, reg: gp21shift, asm: "SHRL", resultInArg0: true, clobberFlags: true},              // unsigned uint32(arg0) >> arg1, shift amount is mod 32
+               {name: "SHRW", argLength: 2, reg: gp21shift, asm: "SHRW", resultInArg0: true, clobberFlags: true},              // unsigned uint16(arg0) >> arg1, shift amount is mod 32
+               {name: "SHRB", argLength: 2, reg: gp21shift, asm: "SHRB", resultInArg0: true, clobberFlags: true},              // unsigned uint8(arg0) >> arg1, shift amount is mod 32
                {name: "SHRQconst", argLength: 1, reg: gp11, asm: "SHRQ", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned arg0 >> auxint, shift amount 0-63
-               {name: "SHRLconst", argLength: 1, reg: gp11, asm: "SHRL", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned arg0 >> auxint, shift amount 0-31
-               {name: "SHRWconst", argLength: 1, reg: gp11, asm: "SHRW", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned arg0 >> auxint, shift amount 0-15
-               {name: "SHRBconst", argLength: 1, reg: gp11, asm: "SHRB", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned arg0 >> auxint, shift amount 0-7
+               {name: "SHRLconst", argLength: 1, reg: gp11, asm: "SHRL", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned uint32(arg0) >> auxint, shift amount 0-31
+               {name: "SHRWconst", argLength: 1, reg: gp11, asm: "SHRW", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned uint16(arg0) >> auxint, shift amount 0-15
+               {name: "SHRBconst", argLength: 1, reg: gp11, asm: "SHRB", aux: "Int8", resultInArg0: true, clobberFlags: true}, // unsigned uint8(arg0) >> auxint, shift amount 0-7
 
                {name: "SARQ", argLength: 2, reg: gp21shift, asm: "SARQ", resultInArg0: true, clobberFlags: true},              // signed arg0 >> arg1, shift amount is mod 64
-               {name: "SARL", argLength: 2, reg: gp21shift, asm: "SARL", resultInArg0: true, clobberFlags: true},              // signed arg0 >> arg1, shift amount is mod 32
-               {name: "SARW", argLength: 2, reg: gp21shift, asm: "SARW", resultInArg0: true, clobberFlags: true},              // signed arg0 >> arg1, shift amount is mod 32
-               {name: "SARB", argLength: 2, reg: gp21shift, asm: "SARB", resultInArg0: true, clobberFlags: true},              // signed arg0 >> arg1, shift amount is mod 32
+               {name: "SARL", argLength: 2, reg: gp21shift, asm: "SARL", resultInArg0: true, clobberFlags: true},              // signed int32(arg0) >> arg1, shift amount is mod 32
+               {name: "SARW", argLength: 2, reg: gp21shift, asm: "SARW", resultInArg0: true, clobberFlags: true},              // signed int16(arg0) >> arg1, shift amount is mod 32
+               {name: "SARB", argLength: 2, reg: gp21shift, asm: "SARB", resultInArg0: true, clobberFlags: true},              // signed int8(arg0) >> arg1, shift amount is mod 32
                {name: "SARQconst", argLength: 1, reg: gp11, asm: "SARQ", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed arg0 >> auxint, shift amount 0-63
-               {name: "SARLconst", argLength: 1, reg: gp11, asm: "SARL", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed arg0 >> auxint, shift amount 0-31
-               {name: "SARWconst", argLength: 1, reg: gp11, asm: "SARW", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed arg0 >> auxint, shift amount 0-15
-               {name: "SARBconst", argLength: 1, reg: gp11, asm: "SARB", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed arg0 >> auxint, shift amount 0-7
+               {name: "SARLconst", argLength: 1, reg: gp11, asm: "SARL", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed int32(arg0) >> auxint, shift amount 0-31
+               {name: "SARWconst", argLength: 1, reg: gp11, asm: "SARW", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed int16(arg0) >> auxint, shift amount 0-15
+               {name: "SARBconst", argLength: 1, reg: gp11, asm: "SARB", aux: "Int8", resultInArg0: true, clobberFlags: true}, // signed int8(arg0) >> auxint, shift amount 0-7
 
                {name: "ROLQ", argLength: 2, reg: gp21shift, asm: "ROLQ", resultInArg0: true, clobberFlags: true},              // arg0 rotate left arg1 bits.
                {name: "ROLL", argLength: 2, reg: gp21shift, asm: "ROLL", resultInArg0: true, clobberFlags: true},              // arg0 rotate left arg1 bits.

diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go
index 9213616f83b6560cb30f5c5b3fba3a076f7875f6..379ea55b88b92359ca9b545d0ad425941a8c2675 100644 (file)
--- a/src/cmd/compile/internal/ssa/rewriteAMD64.go
+++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go
@@ -33234,7 +33234,7 @@ func rewriteValueAMD64_OpAMD64SARBconst_0(v *Value) bool {
        }
        // match: (SARBconst [c] (MOVQconst [d]))
        // cond:
-       // result: (MOVQconst [d>>uint64(c)])
+       // result: (MOVQconst [int64(int8(d))>>uint64(c)])
        for {
                c := v.AuxInt
                v_0 := v.Args[0]
@@ -33243,7 +33243,7 @@ func rewriteValueAMD64_OpAMD64SARBconst_0(v *Value) bool {
                }
                d := v_0.AuxInt
                v.reset(OpAMD64MOVQconst)
-               v.AuxInt = d >> uint64(c)
+               v.AuxInt = int64(int8(d)) >> uint64(c)
                return true
        }
        return false
@@ -33489,7 +33489,7 @@ func rewriteValueAMD64_OpAMD64SARLconst_0(v *Value) bool {
        }
        // match: (SARLconst [c] (MOVQconst [d]))
        // cond:
-       // result: (MOVQconst [d>>uint64(c)])
+       // result: (MOVQconst [int64(int32(d))>>uint64(c)])
        for {
                c := v.AuxInt
                v_0 := v.Args[0]
@@ -33498,7 +33498,7 @@ func rewriteValueAMD64_OpAMD64SARLconst_0(v *Value) bool {
                }
                d := v_0.AuxInt
                v.reset(OpAMD64MOVQconst)
-               v.AuxInt = d >> uint64(c)
+               v.AuxInt = int64(int32(d)) >> uint64(c)
                return true
        }
        return false
@@ -33809,7 +33809,7 @@ func rewriteValueAMD64_OpAMD64SARWconst_0(v *Value) bool {
        }
        // match: (SARWconst [c] (MOVQconst [d]))
        // cond:
-       // result: (MOVQconst [d>>uint64(c)])
+       // result: (MOVQconst [int64(int16(d))>>uint64(c)])
        for {
                c := v.AuxInt
                v_0 := v.Args[0]
@@ -33818,7 +33818,7 @@ func rewriteValueAMD64_OpAMD64SARWconst_0(v *Value) bool {
                }
                d := v_0.AuxInt
                v.reset(OpAMD64MOVQconst)
-               v.AuxInt = d >> uint64(c)
+               v.AuxInt = int64(int16(d)) >> uint64(c)
                return true
        }
        return false

diff --git a/test/fixedbugs/issue23812.go b/test/fixedbugs/issue23812.go
new file mode 100644 (file)
index 0000000..0a40deb
--- /dev/null
+++ b/test/fixedbugs/issue23812.go
@@ -0,0 +1,34 @@
+// 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.
+
+package main
+
+import "fmt"
+
+func main() {
+       want := int32(0x3edae8)
+       got := foo(1)
+       if want != got {
+               panic(fmt.Sprintf("want %x, got %x", want, got))
+       }
+}
+
+func foo(a int32) int32 {
+       return shr1(int32(shr2(int64(0x14ff6e2207db5d1f), int(a))), 4)
+}
+
+func shr1(n int32, m int) int32 { return n >> uint(m) }
+
+func shr2(n int64, m int) int64 {
+       if m < 0 {
+               m = -m
+       }
+       if m >= 64 {
+               return n
+       }
+
+       return n >> uint(m)
+}