cmd/compile: factor out code to remove phi argument

CL 358117 fixed a bug that Phi's argument wasn't updated correctly after
removing a predecessor of Block. This CL factor out the code that
updates phi argument into a Block's method, so it's easier to use,
maintain and hopefully prevent that kind of bug in the future.

Change-Id: Ie9741e19ea28f56860425089b6093a381aa10f5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/357964
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/ssa/block.go b/src/cmd/compile/internal/ssa/block.go
index 71ca774431e33cb99a4344d1aaa6e341a052997a..6ff3188f9b20159bcd71cefd626c1221e03d40ef 100644 (file)
--- a/src/cmd/compile/internal/ssa/block.go
+++ b/src/cmd/compile/internal/ssa/block.go
@@ -279,7 +279,8 @@ func (b *Block) AddEdgeTo(c *Block) {
 
 // removePred removes the ith input edge from b.
 // It is the responsibility of the caller to remove
-// the corresponding successor edge.
+// the corresponding successor edge, and adjust any
+// phi values by calling b.removePhiArg(v, i).
 func (b *Block) removePred(i int) {
        n := len(b.Preds) - 1
        if i != n {
@@ -322,6 +323,28 @@ func (b *Block) swapSuccessors() {
        b.Likely *= -1
 }
 
+// removePhiArg removes the ith arg from phi.
+// It must be called after calling b.removePred(i) to
+// adjust the corresponding phi value of the block:
+//
+// b.removePred(i)
+// for _, v := range b.Values {
+//     if v.Op != OpPhi {
+//         continue
+//     }
+//     b.removeArg(v, i)
+// }
+func (b *Block) removePhiArg(phi *Value, i int) {
+       n := len(b.Preds)
+       if numPhiArgs := len(phi.Args); numPhiArgs-1 != n {
+               b.Fatalf("inconsistent state, num predecessors: %d, num phi args: %d", n, numPhiArgs)
+       }
+       phi.Args[i].Uses--
+       phi.Args[i] = phi.Args[n]
+       phi.Args[n] = nil
+       phi.Args = phi.Args[:n]
+}
+
 // LackingPos indicates whether b is a block whose position should be inherited
 // from its successors.  This is true if all the values within it have unreliable positions
 // and if it is "plain", meaning that there is no control flow that is also very likely

diff --git a/src/cmd/compile/internal/ssa/critical.go b/src/cmd/compile/internal/ssa/critical.go
index b85721eba488323fa1ce39c6aeb8707157f4e832..500ce3ae61ce093de51b3c703160d042db2121ca 100644 (file)
--- a/src/cmd/compile/internal/ssa/critical.go
+++ b/src/cmd/compile/internal/ssa/critical.go
@@ -91,14 +91,13 @@ func critical(f *Func) {
                                b.removePred(i)
 
                                // Update corresponding phi args
-                               n := len(b.Preds)
-                               phi.Args[i].Uses--
-                               phi.Args[i] = phi.Args[n]
-                               phi.Args[n] = nil
-                               phi.Args = phi.Args[:n]
+                               b.removePhiArg(phi, i)
+
                                // splitting occasionally leads to a phi having
                                // a single argument (occurs with -N)
-                               if n == 1 {
+                               // TODO(cuonglm,khr): replace this with phielimValue, and
+                               //                    make removePhiArg incorporates that.
+                               if len(b.Preds) == 1 {
                                        phi.Op = OpCopy
                                }
                                // Don't increment i in this case because we moved

diff --git a/src/cmd/compile/internal/ssa/deadcode.go b/src/cmd/compile/internal/ssa/deadcode.go
index 5d10dfe025e97ccd2dcad6af9500eeb04f7df40f..b47b106975ac13f36b4b14784019dcdd1f673cf1 100644 (file)
--- a/src/cmd/compile/internal/ssa/deadcode.go
+++ b/src/cmd/compile/internal/ssa/deadcode.go
@@ -348,15 +348,11 @@ func (b *Block) removeEdge(i int) {
        c.removePred(j)
 
        // Remove phi args from c's phis.
-       n := len(c.Preds)
        for _, v := range c.Values {
                if v.Op != OpPhi {
                        continue
                }
-               v.Args[j].Uses--
-               v.Args[j] = v.Args[n]
-               v.Args[n] = nil
-               v.Args = v.Args[:n]
+               c.removePhiArg(v, j)
                phielimValue(v)
                // Note: this is trickier than it looks. Replacing
                // a Phi with a Copy can in general cause problems because

diff --git a/src/cmd/compile/internal/ssa/fuse_branchredirect.go b/src/cmd/compile/internal/ssa/fuse_branchredirect.go
index ba5220bd87ca2ee64b4befc8da437c093076491d..751dca7468bc748af777e83d3565511e002473d6 100644 (file)
--- a/src/cmd/compile/internal/ssa/fuse_branchredirect.go
+++ b/src/cmd/compile/internal/ssa/fuse_branchredirect.go
@@ -78,11 +78,7 @@ func fuseBranchRedirect(f *Func) bool {
                                        if v.Op != OpPhi {
                                                continue
                                        }
-                                       n := len(v.Args)
-                                       v.Args[k].Uses--
-                                       v.Args[k] = v.Args[n-1]
-                                       v.Args[n-1] = nil
-                                       v.Args = v.Args[:n-1]
+                                       b.removePhiArg(v, k)
                                        phielimValue(v)
                                }
                                // Fix up child to have one more predecessor.

diff --git a/src/cmd/compile/internal/ssa/shortcircuit.go b/src/cmd/compile/internal/ssa/shortcircuit.go
index 29abf3c591f902748ef2affd7869d8636e053d12..c0b9eacf4174d8bbb80784d7fb026a87e0bc9835 100644 (file)
--- a/src/cmd/compile/internal/ssa/shortcircuit.go
+++ b/src/cmd/compile/internal/ssa/shortcircuit.go
@@ -196,11 +196,7 @@ func shortcircuitBlock(b *Block) bool {
 
        // Remove b's incoming edge from p.
        b.removePred(cidx)
-       n := len(b.Preds)
-       ctl.Args[cidx].Uses--
-       ctl.Args[cidx] = ctl.Args[n]
-       ctl.Args[n] = nil
-       ctl.Args = ctl.Args[:n]
+       b.removePhiArg(ctl, cidx)
 
        // Redirect p's outgoing edge to t.
        p.Succs[pi] = Edge{t, len(t.Preds)}