From ce68047f96c8a35d2c3598a41efbc749964e6b1c Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Tue, 13 Feb 2018 12:33:55 -0800 Subject: [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 TryBot-Result: Gobot Gobot Reviewed-by: Cherry Zhang Reviewed-on: https://go-review.googlesource.com/102775 Run-TryBot: Andrew Bonventre --- src/cmd/compile/internal/ssa/gen/AMD64.rules | 6 ++--- src/cmd/compile/internal/ssa/gen/AMD64Ops.go | 24 ++++++++++---------- src/cmd/compile/internal/ssa/rewriteAMD64.go | 12 +++++----- test/fixedbugs/issue23812.go | 34 ++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 21 deletions(-) create mode 100644 test/fixedbugs/issue23812.go diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules index ff38be550e..89d082ac7f 100644 --- 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 c984cbfb12..12757a7d5a 100644 --- 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 9213616f83..379ea55b88 100644 --- 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 index 0000000000..0a40deb212 --- /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) +} -- cgit v1.2.3-54-g00ecf