[release-branch.go1.7] cmd/compile: compare size in dead store elimination
This CL is a manual backpatch of CL 27320 into the 1.7.1 release branch.
The manual backpatch is required because OpZero changed from having a
size as its AuxInt to having a size+align as its AuxInt (that was to support
the ARM SSA backend). Otherwise the CLs should be identical.
Please review carefully!
Change-Id: I569b759c06d1971c9c62dc5dd589abc7ef7c844a
Reviewed-on: https://go-review.googlesource.com/28670
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/src/cmd/compile/internal/ssa/deadstore.go b/src/cmd/compile/internal/ssa/deadstore.go
index 5129c17..59ac1b9 100644
--- a/src/cmd/compile/internal/ssa/deadstore.go
+++ b/src/cmd/compile/internal/ssa/deadstore.go
@@ -14,8 +14,7 @@
defer f.retSparseSet(loadUse)
storeUse := f.newSparseSet(f.NumValues())
defer f.retSparseSet(storeUse)
- shadowed := f.newSparseSet(f.NumValues())
- defer f.retSparseSet(shadowed)
+ shadowed := newSparseMap(f.NumValues()) // TODO: cache
for _, b := range f.Blocks {
// Find all the stores in this block. Categorize their uses:
// loadUse contains stores which are used by a subsequent load.
@@ -81,17 +80,18 @@
shadowed.clear()
}
if v.Op == OpStore || v.Op == OpZero {
- if shadowed.contains(v.Args[0].ID) {
+ sz := v.AuxInt
+ if shadowedSize := int64(shadowed.get(v.Args[0].ID)); shadowedSize != -1 && shadowedSize >= sz {
// Modify store into a copy
if v.Op == OpStore {
// store addr value mem
v.SetArgs1(v.Args[2])
} else {
// zero addr mem
- sz := v.Args[0].Type.ElemType().Size()
- if v.AuxInt != sz {
+ typesz := v.Args[0].Type.ElemType().Size()
+ if sz != typesz {
f.Fatalf("mismatched zero/store sizes: %d and %d [%s]",
- v.AuxInt, sz, v.LongString())
+ sz, typesz, v.LongString())
}
v.SetArgs1(v.Args[1])
}
@@ -99,7 +99,10 @@
v.AuxInt = 0
v.Op = OpCopy
} else {
- shadowed.add(v.Args[0].ID)
+ if sz > 0x7fffffff { // work around sparseMap's int32 value type
+ sz = 0x7fffffff
+ }
+ shadowed.set(v.Args[0].ID, int32(sz))
}
}
// walk to previous store
diff --git a/src/cmd/compile/internal/ssa/deadstore_test.go b/src/cmd/compile/internal/ssa/deadstore_test.go
index c38f1cd..beb6de0 100644
--- a/src/cmd/compile/internal/ssa/deadstore_test.go
+++ b/src/cmd/compile/internal/ssa/deadstore_test.go
@@ -8,7 +8,7 @@
func TestDeadStore(t *testing.T) {
c := testConfig(t)
- elemType := &TypeImpl{Size_: 8, Name: "testtype"}
+ elemType := &TypeImpl{Size_: 1, Name: "testtype"}
ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr", Elem_: elemType} // dummy for testing
fun := Fun(c, "entry",
Bloc("entry",
@@ -18,7 +18,7 @@
Valu("addr1", OpAddr, ptrType, 0, nil, "sb"),
Valu("addr2", OpAddr, ptrType, 0, nil, "sb"),
Valu("addr3", OpAddr, ptrType, 0, nil, "sb"),
- Valu("zero1", OpZero, TypeMem, 8, nil, "addr3", "start"),
+ Valu("zero1", OpZero, TypeMem, 1, nil, "addr3", "start"),
Valu("store1", OpStore, TypeMem, 1, nil, "addr1", "v", "zero1"),
Valu("store2", OpStore, TypeMem, 1, nil, "addr2", "v", "store1"),
Valu("store3", OpStore, TypeMem, 1, nil, "addr1", "v", "store2"),
@@ -95,3 +95,32 @@
t.Errorf("store %s incorrectly removed", v)
}
}
+
+func TestDeadStoreUnsafe(t *testing.T) {
+ // Make sure a narrow store can't shadow a wider one. The test above
+ // covers the case of two different types, but unsafe pointer casting
+ // can get to a point where the size is changed but type unchanged.
+ c := testConfig(t)
+ ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr"} // dummy for testing
+ fun := Fun(c, "entry",
+ Bloc("entry",
+ Valu("start", OpInitMem, TypeMem, 0, nil),
+ Valu("sb", OpSB, TypeInvalid, 0, nil),
+ Valu("v", OpConstBool, TypeBool, 1, nil),
+ Valu("addr1", OpAddr, ptrType, 0, nil, "sb"),
+ Valu("store1", OpStore, TypeMem, 8, nil, "addr1", "v", "start"), // store 8 bytes
+ Valu("store2", OpStore, TypeMem, 1, nil, "addr1", "v", "store1"), // store 1 byte
+ Goto("exit")),
+ Bloc("exit",
+ Exit("store2")))
+
+ CheckFunc(fun.f)
+ cse(fun.f)
+ dse(fun.f)
+ CheckFunc(fun.f)
+
+ v := fun.values["store1"]
+ if v.Op == OpCopy {
+ t.Errorf("store %s incorrectly removed", v)
+ }
+}
