From 24f46bd34fff99cbf64762b65e7896fd1e8f2f7c Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Wed, 7 Sep 2016 10:57:26 -0700 Subject: [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 Reviewed-by: David Chase Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot --- src/cmd/compile/internal/ssa/deadstore.go | 17 +++++++------ src/cmd/compile/internal/ssa/deadstore_test.go | 33 ++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/cmd/compile/internal/ssa/deadstore.go b/src/cmd/compile/internal/ssa/deadstore.go index 5129c171bb..59ac1b9ad4 100644 --- a/src/cmd/compile/internal/ssa/deadstore.go +++ b/src/cmd/compile/internal/ssa/deadstore.go @@ -14,8 +14,7 @@ func dse(f *Func) { 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 @@ func dse(f *Func) { 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 @@ func dse(f *Func) { 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 c38f1cdbaf..beb6de0637 100644 --- a/src/cmd/compile/internal/ssa/deadstore_test.go +++ b/src/cmd/compile/internal/ssa/deadstore_test.go @@ -8,7 +8,7 @@ import "testing" 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 @@ func TestDeadStore(t *testing.T) { 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 @@ func TestDeadStoreTypes(t *testing.T) { 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) + } +} -- cgit v1.2.3-54-g00ecf