From 4ad643d2089b73fbcfc2c0e3f61cb63dcb217ec5 Mon Sep 17 00:00:00 2001
From: Josh Bleecher Snyder <josharian@gmail.com>
Date: Sun, 8 Mar 2020 11:07:52 -0700
Subject: [PATCH] cmd/compile: remove loop in shortcircuit

shortcircuitBlock contained a loop to handle blocks like

b: <- p q
  v = Phi true false
If v -> t u

in a single execution.
This change makes shortcircuitBlock do it in two instead,
one for each constant phi arg.

Motivation: Upcoming changes will expand the range of
blocks that the shortcircuit pass can handle.
Those changes need to understand what the CFG
will look like after the rewrite in shortcircuitBlock.
Making shortcircuitBlock do only a single CFG
modification at a time significantly simplifies that code.

In theory, this is less efficient, but not measurably so.
There is minor, unimportant churn in the generated code.

Updates #37608

Change-Id: Ia6dce7011e3e19b546ed1e176bd407575a0ab837
Reviewed-on: https://go-review.googlesource.com/c/go/+/222918
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
---
 src/cmd/compile/internal/ssa/shortcircuit.go | 92 ++++++++++----------
 1 file changed, 45 insertions(+), 47 deletions(-)

diff --git a/src/cmd/compile/internal/ssa/shortcircuit.go b/src/cmd/compile/internal/ssa/shortcircuit.go
index 42b0639cae..274ef9a128 100644
--- a/src/cmd/compile/internal/ssa/shortcircuit.go
+++ b/src/cmd/compile/internal/ssa/shortcircuit.go
@@ -117,65 +117,63 @@ func shortcircuitBlock(b *Block) bool {
 		return false
 	}
 
-	// Check for const phi args.
-	var changed bool
-	for i := 0; i < len(ctl.Args); i++ {
-		a := ctl.Args[i]
-		if a.Op != OpConstBool {
-			continue
+	// Locate index of first const phi arg.
+	cidx := -1
+	for i, a := range ctl.Args {
+		if a.Op == OpConstBool {
+			cidx = i
+			break
 		}
-		// The predecessor we come in from.
-		e1 := b.Preds[i]
-		p := e1.b
-		pi := e1.i
+	}
+	if cidx == -1 {
+		return false
+	}
 
-		// The successor we always go to when coming in
-		// from that predecessor.
-		si := 1 - a.AuxInt
-		if swap {
-			si = 1 - si
-		}
-		e2 := b.Succs[si]
-		t := e2.b
-		if p == b || t == b {
-			// This is an infinite loop; we can't remove it. See issue 33903.
-			continue
-		}
-		ti := e2.i
+	a := ctl.Args[cidx]
+	// The predecessor we come in from.
+	e1 := b.Preds[cidx]
+	p := e1.b
+	pi := e1.i
 
-		// Update CFG and Phis.
-		changed = true
+	// The successor we always go to when coming in
+	// from that predecessor.
+	si := 1 - a.AuxInt
+	if swap {
+		si = 1 - si
+	}
+	e2 := b.Succs[si]
+	t := e2.b
+	if p == b || t == b {
+		// This is an infinite loop; we can't remove it. See issue 33903.
+		return false
+	}
+	ti := e2.i
 
-		// Remove b's incoming edge from p.
-		b.removePred(i)
-		n := len(b.Preds)
-		ctl.Args[i].Uses--
-		ctl.Args[i] = ctl.Args[n]
-		ctl.Args[n] = nil
-		ctl.Args = ctl.Args[:n]
+	// We're committed. Update CFG and Phis.
 
-		// Redirect p's outgoing edge to t.
-		p.Succs[pi] = Edge{t, len(t.Preds)}
+	// 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]
 
-		// Fix up t to have one more predecessor.
-		t.Preds = append(t.Preds, Edge{p, pi})
-		for _, w := range t.Values {
-			if w.Op != OpPhi {
-				continue
-			}
-			w.AddArg(w.Args[ti])
-		}
-		i--
-	}
+	// Redirect p's outgoing edge to t.
+	p.Succs[pi] = Edge{t, len(t.Preds)}
 
-	if !changed {
-		return false
+	// Fix up t to have one more predecessor.
+	t.Preds = append(t.Preds, Edge{p, pi})
+	for _, w := range t.Values {
+		if w.Op != OpPhi {
+			continue
+		}
+		w.AddArg(w.Args[ti])
 	}
 
 	if len(b.Preds) == 0 {
 		// Block is now dead.
 		b.Kind = BlockInvalid
-		return true
 	}
 
 	phielimValue(ctl)
-- 
2.50.0