From: Cherry Zhang Date: Fri, 5 Mar 2021 20:12:37 +0000 (-0500) Subject: cmd/compile: match Aux and AuxInt explicitly in store combining rule X-Git-Tag: go1.17beta1~1249 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=a829114b21;p=gostls13.git cmd/compile: match Aux and AuxInt explicitly in store combining rule CL 280456 introduced a new store combining rule. On the LHS some of the Aux and AuxInt of the stores are not specified, therefore ignored during the matching. The rule is only correct if they match. This CL adds explict match. TODO: maybe we want the rule matcher require Aux/AuxInt to be always specified on the LHS (using _ to explicitly ignore)? Or maybe we want it to match the zero value if not specified? The current approach is error-prone. Fixes #44823. Change-Id: Ic12b4a0de63117f2f070039737f0c905f28561bc Reviewed-on: https://go-review.googlesource.com/c/go/+/299289 Trust: Cherry Zhang Trust: Josh Bleecher Snyder Reviewed-by: Josh Bleecher Snyder --- diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules index bab9cee88c..7b03034bb7 100644 --- a/src/cmd/compile/internal/ssa/gen/AMD64.rules +++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules @@ -1970,15 +1970,15 @@ && clobber(x) => (MOVQstore [i] {s} p0 w0 mem) -(MOVBstore [7] p1 (SHRQconst [56] w) - x1:(MOVWstore [5] p1 (SHRQconst [40] w) - x2:(MOVLstore [1] p1 (SHRQconst [8] w) - x3:(MOVBstore p1 w mem)))) +(MOVBstore [7] {s} p1 (SHRQconst [56] w) + x1:(MOVWstore [5] {s} p1 (SHRQconst [40] w) + x2:(MOVLstore [1] {s} p1 (SHRQconst [8] w) + x3:(MOVBstore [0] {s} p1 w mem)))) && x1.Uses == 1 && x2.Uses == 1 && x3.Uses == 1 && clobber(x1, x2, x3) - => (MOVQstore p1 w mem) + => (MOVQstore {s} p1 w mem) (MOVBstore [i] {s} p x1:(MOVBload [j] {s2} p2 mem) diff --git a/src/cmd/compile/internal/ssa/gen/S390X.rules b/src/cmd/compile/internal/ssa/gen/S390X.rules index e4a1cd6981..1f75f78a71 100644 --- a/src/cmd/compile/internal/ssa/gen/S390X.rules +++ b/src/cmd/compile/internal/ssa/gen/S390X.rules @@ -1422,15 +1422,15 @@ && clobber(x) => (MOVDBRstore [i-4] {s} p w0 mem) -(MOVBstore [7] p1 (SRDconst w) - x1:(MOVHBRstore [5] p1 (SRDconst w) - x2:(MOVWBRstore [1] p1 (SRDconst w) - x3:(MOVBstore p1 w mem)))) +(MOVBstore [7] {s} p1 (SRDconst w) + x1:(MOVHBRstore [5] {s} p1 (SRDconst w) + x2:(MOVWBRstore [1] {s} p1 (SRDconst w) + x3:(MOVBstore [0] {s} p1 w mem)))) && x1.Uses == 1 && x2.Uses == 1 && x3.Uses == 1 && clobber(x1, x2, x3) - => (MOVDBRstore p1 w mem) + => (MOVDBRstore {s} p1 w mem) // Combining byte loads into larger (unaligned) loads. diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go index 52d0fd095d..8da3b28b5c 100644 --- a/src/cmd/compile/internal/ssa/rewriteAMD64.go +++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go @@ -11415,20 +11415,21 @@ func rewriteValueAMD64_OpAMD64MOVBstore(v *Value) bool { v.AddArg3(p0, w0, mem) return true } - // match: (MOVBstore [7] p1 (SHRQconst [56] w) x1:(MOVWstore [5] p1 (SHRQconst [40] w) x2:(MOVLstore [1] p1 (SHRQconst [8] w) x3:(MOVBstore p1 w mem)))) + // match: (MOVBstore [7] {s} p1 (SHRQconst [56] w) x1:(MOVWstore [5] {s} p1 (SHRQconst [40] w) x2:(MOVLstore [1] {s} p1 (SHRQconst [8] w) x3:(MOVBstore [0] {s} p1 w mem)))) // cond: x1.Uses == 1 && x2.Uses == 1 && x3.Uses == 1 && clobber(x1, x2, x3) - // result: (MOVQstore p1 w mem) + // result: (MOVQstore {s} p1 w mem) for { if auxIntToInt32(v.AuxInt) != 7 { break } + s := auxToSym(v.Aux) p1 := v_0 if v_1.Op != OpAMD64SHRQconst || auxIntToInt8(v_1.AuxInt) != 56 { break } w := v_1.Args[0] x1 := v_2 - if x1.Op != OpAMD64MOVWstore || auxIntToInt32(x1.AuxInt) != 5 { + if x1.Op != OpAMD64MOVWstore || auxIntToInt32(x1.AuxInt) != 5 || auxToSym(x1.Aux) != s { break } _ = x1.Args[2] @@ -11440,7 +11441,7 @@ func rewriteValueAMD64_OpAMD64MOVBstore(v *Value) bool { break } x2 := x1.Args[2] - if x2.Op != OpAMD64MOVLstore || auxIntToInt32(x2.AuxInt) != 1 { + if x2.Op != OpAMD64MOVLstore || auxIntToInt32(x2.AuxInt) != 1 || auxToSym(x2.Aux) != s { break } _ = x2.Args[2] @@ -11452,7 +11453,7 @@ func rewriteValueAMD64_OpAMD64MOVBstore(v *Value) bool { break } x3 := x2.Args[2] - if x3.Op != OpAMD64MOVBstore { + if x3.Op != OpAMD64MOVBstore || auxIntToInt32(x3.AuxInt) != 0 || auxToSym(x3.Aux) != s { break } mem := x3.Args[2] @@ -11460,6 +11461,7 @@ func rewriteValueAMD64_OpAMD64MOVBstore(v *Value) bool { break } v.reset(OpAMD64MOVQstore) + v.Aux = symToAux(s) v.AddArg3(p1, w, mem) return true } diff --git a/src/cmd/compile/internal/ssa/rewriteS390X.go b/src/cmd/compile/internal/ssa/rewriteS390X.go index e0a5ff4cbb..85260dace8 100644 --- a/src/cmd/compile/internal/ssa/rewriteS390X.go +++ b/src/cmd/compile/internal/ssa/rewriteS390X.go @@ -8883,20 +8883,21 @@ func rewriteValueS390X_OpS390XMOVBstore(v *Value) bool { v.AddArg3(p, w0, mem) return true } - // match: (MOVBstore [7] p1 (SRDconst w) x1:(MOVHBRstore [5] p1 (SRDconst w) x2:(MOVWBRstore [1] p1 (SRDconst w) x3:(MOVBstore p1 w mem)))) + // match: (MOVBstore [7] {s} p1 (SRDconst w) x1:(MOVHBRstore [5] {s} p1 (SRDconst w) x2:(MOVWBRstore [1] {s} p1 (SRDconst w) x3:(MOVBstore [0] {s} p1 w mem)))) // cond: x1.Uses == 1 && x2.Uses == 1 && x3.Uses == 1 && clobber(x1, x2, x3) - // result: (MOVDBRstore p1 w mem) + // result: (MOVDBRstore {s} p1 w mem) for { if auxIntToInt32(v.AuxInt) != 7 { break } + s := auxToSym(v.Aux) p1 := v_0 if v_1.Op != OpS390XSRDconst { break } w := v_1.Args[0] x1 := v_2 - if x1.Op != OpS390XMOVHBRstore || auxIntToInt32(x1.AuxInt) != 5 { + if x1.Op != OpS390XMOVHBRstore || auxIntToInt32(x1.AuxInt) != 5 || auxToSym(x1.Aux) != s { break } _ = x1.Args[2] @@ -8908,7 +8909,7 @@ func rewriteValueS390X_OpS390XMOVBstore(v *Value) bool { break } x2 := x1.Args[2] - if x2.Op != OpS390XMOVWBRstore || auxIntToInt32(x2.AuxInt) != 1 { + if x2.Op != OpS390XMOVWBRstore || auxIntToInt32(x2.AuxInt) != 1 || auxToSym(x2.Aux) != s { break } _ = x2.Args[2] @@ -8920,7 +8921,7 @@ func rewriteValueS390X_OpS390XMOVBstore(v *Value) bool { break } x3 := x2.Args[2] - if x3.Op != OpS390XMOVBstore { + if x3.Op != OpS390XMOVBstore || auxIntToInt32(x3.AuxInt) != 0 || auxToSym(x3.Aux) != s { break } mem := x3.Args[2] @@ -8928,6 +8929,7 @@ func rewriteValueS390X_OpS390XMOVBstore(v *Value) bool { break } v.reset(OpS390XMOVDBRstore) + v.Aux = symToAux(s) v.AddArg3(p1, w, mem) return true } diff --git a/test/fixedbugs/issue44823.go b/test/fixedbugs/issue44823.go new file mode 100644 index 0000000000..85811df67d --- /dev/null +++ b/test/fixedbugs/issue44823.go @@ -0,0 +1,26 @@ +// run + +// Copyright 2021 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. + +// Issue 44823: miscompilation with store combining. + +package main + +import "encoding/binary" + +//go:noinline +func Id(a [8]byte) (x [8]byte) { + binary.LittleEndian.PutUint64(x[:], binary.LittleEndian.Uint64(a[:])) + return +} + +var a = [8]byte{1, 2, 3, 4, 5, 6, 7, 8} + +func main() { + x := Id(a) + if x != a { + panic("FAIL") + } +}