diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go index 16cb04df98..0c3cb3e294 100644 --- a/src/cmd/compile/internal/ssa/nilcheck.go +++ b/src/cmd/compile/internal/ssa/nilcheck.go @@ -38,7 +38,7 @@ func nilcheckelim(f *Func) { } work := make([]bp, 0, 256) - work = append(work, bp{block: f.Entry, ptr: checkedptr(f.Entry)}) + work = append(work, bp{block: f.Entry}) // map from value ID to bool indicating if value is known to be non-nil // in the current dominator path being walked. This slice is updated by @@ -74,27 +74,38 @@ func nilcheckelim(f *Func) { node := work[len(work)-1] work = work[:len(work)-1] - var pushRecPtr bool switch node.op { case Work: - if node.ptr != nil { - // already have a nilcheck in the dominator path - if nonNilValues[node.ptr.ID] { + checked := checkedptr(node.block) // ptr being checked for nil/non-nil + nonnil := nonnilptr(node.block) // ptr that is non-nil due to this blocks pred + + if checked != nil { + // already have a nilcheck in the dominator path, or this block is a success + // block for the same value it is checking + if nonNilValues[checked.ID] || checked == nonnil { // Eliminate the nil check. // The deadcode pass will remove vestigial values, // and the fuse pass will join this block with its successor. node.block.Kind = BlockFirst node.block.Control = nil - } else { - // new nilcheck so add a ClearPtr node to clear the - // ptr from the map of nil checks once we traverse - // back up the tree - work = append(work, bp{op: ClearPtr, ptr: node.ptr}) - // and cause a new setPtr to be appended after the - // block's dominees - pushRecPtr = true } } + + if nonnil != nil && !nonNilValues[nonnil.ID] { + // this is a new nilcheck so add a ClearPtr node to clear the + // ptr from the map of nil checks once we traverse + // back up the tree + work = append(work, bp{op: ClearPtr, ptr: nonnil}) + } + + // add all dominated blocks to the work list + for _, w := range domTree[node.block.ID] { + work = append(work, bp{block: w}) + } + + if nonnil != nil && !nonNilValues[nonnil.ID] { + work = append(work, bp{op: RecPtr, ptr: nonnil}) + } case RecPtr: nonNilValues[node.ptr.ID] = true continue @@ -102,77 +113,6 @@ func nilcheckelim(f *Func) { nonNilValues[node.ptr.ID] = false continue } - - var nilBranch *Block - for _, w := range domTree[node.block.ID] { - // We are about to traverse down the 'ptr is nil' side - // of a nilcheck block, so save it for later. This doesn't - // remove nil checks on the false side of the OpIsNonNil branch. - // This is important otherwise we would remove nil checks that - // are not redundant. - if node.block.Kind == BlockIf && node.block.Control.Op == OpIsNonNil && - w == node.block.Succs[1] { - nilBranch = w - continue - } - work = append(work, bp{block: w, ptr: checkedptr(w)}) - } - - if nilBranch != nil { - // we pop from the back of the work slice, so this sets - // up the false branch to be operated on before the - // node.ptr is recorded - work = append(work, bp{op: RecPtr, ptr: node.ptr}) - work = append(work, bp{block: nilBranch, ptr: checkedptr(nilBranch)}) - } else if pushRecPtr { - work = append(work, bp{op: RecPtr, ptr: node.ptr}) - } - } -} - -// nilcheckelim0 is the original redundant nilcheck elimination algorithm. -func nilcheckelim0(f *Func) { - // Exit early if there are no nil checks to eliminate. - var found bool - for _, b := range f.Blocks { - if checkedptr(b) != nil { - found = true - break - } - } - if !found { - return - } - - // Eliminate redundant nil checks. - // A nil check is redundant if the same - // nil check has been performed by a - // dominating block. - // The efficacy of this pass depends - // heavily on the efficacy of the cse pass. - idom := dominators(f) // TODO: cache the dominator tree in the function, clearing when the CFG changes? - for _, b := range f.Blocks { - ptr := checkedptr(b) - if ptr == nil { - continue - } - var elim bool - // Walk up the dominator tree, - // looking for identical nil checks. - // TODO: This loop is O(n^2). See BenchmarkNilCheckDeep*. - for c := idom[b.ID]; c != nil; c = idom[c.ID] { - if checkedptr(c) == ptr { - elim = true - break - } - } - if elim { - // Eliminate the nil check. - // The deadcode pass will remove vestigial values, - // and the fuse pass will join this block with its successor. - b.Kind = BlockFirst - b.Control = nil - } } } @@ -184,3 +124,17 @@ func checkedptr(b *Block) *Value { } return nil } + +// nonnilptr returns the Value, if any, +// that is non-nil due to b being the success block +// of an OpIsNonNil block for the value and having a single +// predecessor. +func nonnilptr(b *Block) *Value { + if len(b.Preds) == 1 { + bp := b.Preds[0] + if bp.Kind == BlockIf && bp.Control.Op == OpIsNonNil && bp.Succs[0] == b { + return bp.Control.Args[0] + } + } + return nil +} diff --git a/src/cmd/compile/internal/ssa/nilcheck_test.go b/src/cmd/compile/internal/ssa/nilcheck_test.go index 1d048fbb34..cbd17e0093 100644 --- a/src/cmd/compile/internal/ssa/nilcheck_test.go +++ b/src/cmd/compile/internal/ssa/nilcheck_test.go @@ -382,3 +382,48 @@ func TestNilcheckUser(t *testing.T) { } } } + +// TestNilcheckBug reproduces a bug in nilcheckelim found by compiling math/big +func TestNilcheckBug(t *testing.T) { + ptrType := &TypeImpl{Size_: 8, Ptr: true, Name: "testptr"} // dummy for testing + c := NewConfig("amd64", DummyFrontend{t}) + fun := Fun(c, "entry", + Bloc("entry", + Valu("mem", OpArg, TypeMem, 0, ".mem"), + Valu("sb", OpSB, TypeInvalid, 0, nil), + Goto("checkPtr")), + Bloc("checkPtr", + Valu("ptr1", OpConstPtr, ptrType, 0, nil, "sb"), + Valu("nilptr", OpConstNil, ptrType, 0, nil, "sb"), + Valu("bool1", OpNeqPtr, TypeBool, 0, nil, "ptr1", "nilptr"), + If("bool1", "secondCheck", "couldBeNil")), + Bloc("couldBeNil", + Goto("secondCheck")), + Bloc("secondCheck", + Valu("bool2", OpIsNonNil, TypeBool, 0, nil, "ptr1"), + If("bool2", "extra", "exit")), + Bloc("extra", + Goto("exit")), + Bloc("exit", + Exit("mem"))) + + CheckFunc(fun.f) + // we need the opt here to rewrite the user nilcheck + opt(fun.f) + nilcheckelim(fun.f) + + // clean up the removed nil check + fuse(fun.f) + deadcode(fun.f) + + CheckFunc(fun.f) + foundSecondCheck := false + for _, b := range fun.f.Blocks { + if b == fun.blocks["secondCheck"] && isNilCheck(b) { + foundSecondCheck = true + } + } + if !foundSecondCheck { + t.Errorf("secondCheck was eliminated, but shouldn't have") + } +} diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go old mode 100755 new mode 100644 index 4cc181f610..d80547ed1c --- a/src/cmd/dist/test.go +++ b/src/cmd/dist/test.go @@ -281,9 +281,6 @@ func (t *tester) registerSSATest(pkg string) { // known failures due to GOGC=off case "runtime", "runtime/pprof", "runtime/trace", "sync": return - // TODO: fix these failures - case "math/big", "cmd/compile/internal/big": - return } t.tests = append(t.tests, distTest{ name: "go_test_ssa:" + pkg,