[dev.ssa] cmd/compile/ssa: handle nested dead blocks

removePredecessor can change which blocks are live.
However, it cannot remove dead blocks from the function's
slice of blocks because removePredecessor may have been
called from within a function doing a walk of the blocks.

CL 11879 did not handle this correctly and broke the build.

To fix this, mark the block as dead but leave its actual
removal for a deadcode pass. Blocks that are dead must have
no successors, predecessors, values, or control values,
so they will generally be ignored by other passes.
To be safe, we add a deadcode pass after the opt pass,
which is the only other pass that calls removePredecessor.

Two alternatives that I considered and discarded:

(1) Make all call sites aware of the fact that removePrecessor
might make arbitrary changes to the list of blocks. This
will needlessly complicate callers.

(2) Handle the things that can go wrong in practice when
we encounter a dead-but-not-removed block. CL 11930 takes
this approach (and the tests are stolen from that CL).
However, this is just patching over the problem.

Change-Id: Icf0687b0a8148ce5e96b2988b668804411b05bd8
Reviewed-on: https://go-review.googlesource.com/12004
Reviewed-by: Todd Neal <todd@tneal.org>
Reviewed-by: Michael Matloob <michaelmatloob@gmail.com>

diff --git a/src/cmd/compile/internal/ssa/check.go b/src/cmd/compile/internal/ssa/check.go
index a27e1bc653cdb79a59ef3553c700054a00072fe7..4fe59e08d1bb29afcf1a302e433146c77d3f135a 100644 (file)
--- a/src/cmd/compile/internal/ssa/check.go
+++ b/src/cmd/compile/internal/ssa/check.go
@@ -59,6 +59,19 @@ func checkFunc(f *Func) {
                        if !b.Control.Type.IsMemory() {
                                f.Fatalf("exit block %s has non-memory control value %s", b, b.Control.LongString())
                        }
+               case BlockDead:
+                       if len(b.Succs) != 0 {
+                               f.Fatalf("dead block %s has successors", b)
+                       }
+                       if len(b.Preds) != 0 {
+                               f.Fatalf("dead block %s has predecessors", b)
+                       }
+                       if len(b.Values) != 0 {
+                               f.Fatalf("dead block %s has values", b)
+                       }
+                       if b.Control != nil {
+                               f.Fatalf("dead block %s has a control value", b)
+                       }
                case BlockPlain:
                        if len(b.Succs) != 1 {
                                f.Fatalf("plain block %s len(Succs)==%d, want 1", b, len(b.Succs))

diff --git a/src/cmd/compile/internal/ssa/compile.go b/src/cmd/compile/internal/ssa/compile.go
index b02c10a745a8c78a3682ee06c2025fb589bc2337..4a6c2a940495208334249fab02dccace8caf4b74 100644 (file)
--- a/src/cmd/compile/internal/ssa/compile.go
+++ b/src/cmd/compile/internal/ssa/compile.go
@@ -51,6 +51,7 @@ var passes = [...]pass{
        {"phielim", phielim},
        {"copyelim", copyelim},
        {"opt", opt},
+       {"opt deadcode", deadcode}, // remove any blocks orphaned during opt
        {"generic cse", cse},
        {"nilcheckelim", nilcheckelim},
        {"generic deadcode", deadcode},

diff --git a/src/cmd/compile/internal/ssa/deadcode.go b/src/cmd/compile/internal/ssa/deadcode.go
index a5d0fe0f34fa8cac4c7d03b6eb99a40ad8a84ea9..2be7b8ebaf5146c26b404a3a526f73faf679dbcc 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadcode.go
+++ b/src/cmd/compile/internal/ssa/deadcode.go
@@ -96,7 +96,7 @@ func deadcode(f *Func) {
        // TODO: save dead Values and Blocks for reuse?  Or should we just let GC handle it?
 }
 
-// There was an edge b->c.  It has been removed from b's successors.
+// There was an edge b->c.  c has been removed from b's successors.
 // Fix up c to handle that fact.
 func (f *Func) removePredecessor(b, c *Block) {
        work := [][2]*Block{{b, c}}
@@ -105,8 +105,6 @@ func (f *Func) removePredecessor(b, c *Block) {
                b, c := work[0][0], work[0][1]
                work = work[1:]
 
-               n := len(c.Preds) - 1
-
                // find index of b in c's predecessor list
                var i int
                for j, p := range c.Preds {
@@ -116,6 +114,7 @@ func (f *Func) removePredecessor(b, c *Block) {
                        }
                }
 
+               n := len(c.Preds) - 1
                c.Preds[i] = c.Preds[n]
                c.Preds[n] = nil // aid GC
                c.Preds = c.Preds[:n]
@@ -143,6 +142,9 @@ func (f *Func) removePredecessor(b, c *Block) {
                        for _, succ := range c.Succs {
                                work = append(work, [2]*Block{c, succ})
                        }
+                       c.Succs = nil
+                       c.Kind = BlockDead
+                       c.Control = nil
                }
        }
 }

diff --git a/src/cmd/compile/internal/ssa/deadcode_test.go b/src/cmd/compile/internal/ssa/deadcode_test.go
index ff9e6800dabb71a4aa44a93c2acb75c87a3252fd..c63b8e4106153b7e7c41b31f3a19134741d63e95 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadcode_test.go
+++ b/src/cmd/compile/internal/ssa/deadcode_test.go
@@ -93,3 +93,42 @@ func TestNeverTaken(t *testing.T) {
        }
 
 }
+
+func TestNestedDeadBlocks(t *testing.T) {
+       c := NewConfig("amd64", DummyFrontend{t})
+       fun := Fun(c, "entry",
+               Bloc("entry",
+                       Valu("mem", OpArg, TypeMem, 0, ".mem"),
+                       Valu("cond", OpConst, TypeBool, 0, false),
+                       If("cond", "b2", "b4")),
+               Bloc("b2",
+                       If("cond", "b3", "b4")),
+               Bloc("b3",
+                       If("cond", "b3", "b4")),
+               Bloc("b4",
+                       If("cond", "b3", "exit")),
+               Bloc("exit",
+                       Exit("mem")))
+
+       CheckFunc(fun.f)
+       Opt(fun.f)
+       CheckFunc(fun.f)
+       Deadcode(fun.f)
+       CheckFunc(fun.f)
+       if fun.blocks["entry"].Kind != BlockPlain {
+               t.Errorf("if(false) not simplified")
+       }
+       for _, b := range fun.f.Blocks {
+               if b == fun.blocks["b2"] {
+                       t.Errorf("b2 block still present")
+               }
+               if b == fun.blocks["b3"] {
+                       t.Errorf("b3 block still present")
+               }
+               for _, v := range b.Values {
+                       if v == fun.values["cond"] {
+                               t.Errorf("constant condition still present")
+                       }
+               }
+       }
+}

diff --git a/src/cmd/compile/internal/ssa/gen/genericOps.go b/src/cmd/compile/internal/ssa/gen/genericOps.go
index a6e6c93fc553427e87420ebeba6e37e768d73373..c410cc4f027d63004fa23b17e27cc3505a406592 100644 (file)
--- a/src/cmd/compile/internal/ssa/gen/genericOps.go
+++ b/src/cmd/compile/internal/ssa/gen/genericOps.go
@@ -105,6 +105,7 @@ var genericOps = []opData{
 
 var genericBlocks = []blockData{
        {name: "Exit"},  // no successors.  There should only be 1 of these.
+       {name: "Dead"},  // no successors; determined to be dead but not yet removed
        {name: "Plain"}, // a single successor
        {name: "If"},    // 2 successors, if control goto Succs[0] else goto Succs[1]
        {name: "Call"},  // 2 successors, normal return and panic

diff --git a/src/cmd/compile/internal/ssa/opGen.go b/src/cmd/compile/internal/ssa/opGen.go
index a6fb0b06e2a2d1ccff15ffafcd0d5f08eaaae06f..3769cfeb867216fb8cdd5a6f9324d68ee9d91083 100644 (file)
--- a/src/cmd/compile/internal/ssa/opGen.go
+++ b/src/cmd/compile/internal/ssa/opGen.go
@@ -19,6 +19,7 @@ const (
        BlockAMD64UGE
 
        BlockExit
+       BlockDead
        BlockPlain
        BlockIf
        BlockCall
@@ -39,6 +40,7 @@ var blockString = [...]string{
        BlockAMD64UGE: "UGE",
 
        BlockExit:  "Exit",
+       BlockDead:  "Dead",
        BlockPlain: "Plain",
        BlockIf:    "If",
        BlockCall:  "Call",

diff --git a/src/cmd/compile/internal/ssa/rewrite.go b/src/cmd/compile/internal/ssa/rewrite.go
index b2c45969e48c1e98b6480c491fa5060cade4283d..306fe1274ee31cd0bafae0a78b2924e5177ba294 100644 (file)
--- a/src/cmd/compile/internal/ssa/rewrite.go
+++ b/src/cmd/compile/internal/ssa/rewrite.go
@@ -23,6 +23,9 @@ func applyRewrite(f *Func, rb func(*Block) bool, rv func(*Value, *Config) bool)
        for {
                change := false
                for _, b := range f.Blocks {
+                       if b.Kind == BlockDead {
+                               continue
+                       }
                        if b.Control != nil && b.Control.Op == OpCopy {
                                for b.Control.Op == OpCopy {
                                        b.Control = b.Control.Args[0]