cmd/compile: fix and improve alias detection

"aliased" is the function responsible for detecting whether we can
turn "a, b = x, y" into just "a = x; b = y", or we need to pre-compute
y and save it in a temporary variable because it might depend on a.

It currently has two issues:

1. It suboptimally treats assignments to blank as writes to heap
   memory. Users generally won't write "_, b = x, y" directly, but it
   comes up a lot in generated code within the compiler.

   This CL changes it to ignore blank assignments.

2. When deciding whether the assigned variable might be referenced by
   pointers, it mistakenly checks Class() and Name.Addrtaken() on "n"
   (the *value* expression being assigned) rather than "a" (the
   destination expression).

   It doesn't appear to result in correctness issues (i.e.,
   incorrectly reporting no aliasing when there is potential aliasing),
   due to all the (overly conservative) rewrite passes before code
   reaches here. But it generates unnecessary code and could have
   correctness issues if we improve those other passes to be more
   aggressive.

   This CL fixes the misuse of "n" for "a" by renaming the variables
   to "r" and "l", respectively, to make their meaning clearer.

Improving these two cases shaves 4.6kB of text from cmd/go, and 93kB
from k8s.io/kubernetes/cmd/kubelet:

       text	   data	    bss	    dec	    hex	filename
    9732136	 290072	 231552	10253760	 9c75c0	go.before
    9727542	 290072	 231552	10249166	 9c63ce	go.after
    97977637	1007051	 301344	99286032	5eafc10	kubelet.before
    97884549	1007051	 301344	99192944	5e99070	kubelet.after

While here, this CL also collapses "memwrite" and "varwrite" into a
single variable. Logically, they're detecting the same thing: are we
assigning to a memory location that a pointer might alias. There's no
need for two variables.

Updates #6853.
Updates #23017.

Change-Id: I5a307b8e20bcd2196e85c55eb025d3f01e303008
Reviewed-on: https://go-review.googlesource.com/c/go/+/261677
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index 3fe7c3e089a2b62ed2b3a808124438c583628edf..0388662a4fd59231ed5e056e0bc2cc770f4765bb 100644 (file)
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -2157,7 +2157,7 @@ func reorder3(all []*Node) []*Node {
 // The result of reorder3save MUST be assigned back to n, e.g.
 //     n.Left = reorder3save(n.Left, all, i, early)
 func reorder3save(n *Node, all []*Node, i int, early *[]*Node) *Node {
-       if !aliased(n, all, i) {
+       if !aliased(n, all[:i]) {
                return n
        }
 
@@ -2189,73 +2189,75 @@ func outervalue(n *Node) *Node {
        }
 }
 
-// Is it possible that the computation of n might be
-// affected by writes in as up to but not including the ith element?
-func aliased(n *Node, all []*Node, i int) bool {
-       if n == nil {
+// Is it possible that the computation of r might be
+// affected by assignments in all?
+func aliased(r *Node, all []*Node) bool {
+       if r == nil {
                return false
        }
 
        // Treat all fields of a struct as referring to the whole struct.
        // We could do better but we would have to keep track of the fields.
-       for n.Op == ODOT {
-               n = n.Left
+       for r.Op == ODOT {
+               r = r.Left
        }
 
        // Look for obvious aliasing: a variable being assigned
        // during the all list and appearing in n.
-       // Also record whether there are any writes to main memory.
-       // Also record whether there are any writes to variables
-       // whose addresses have been taken.
+       // Also record whether there are any writes to addressable
+       // memory (either main memory or variables whose addresses
+       // have been taken).
        memwrite := false
-       varwrite := false
-       for _, an := range all[:i] {
-               a := outervalue(an.Left)
-
-               for a.Op == ODOT {
-                       a = a.Left
+       for _, as := range all {
+               // We can ignore assignments to blank.
+               if as.Left.isBlank() {
+                       continue
                }
 
-               if a.Op != ONAME {
+               l := outervalue(as.Left)
+               if l.Op != ONAME {
                        memwrite = true
                        continue
                }
 
-               switch n.Class() {
+               switch l.Class() {
                default:
-                       varwrite = true
+                       Fatalf("unexpected class: %v, %v", l, l.Class())
+
+               case PAUTOHEAP, PEXTERN:
+                       memwrite = true
                        continue
 
                case PAUTO, PPARAM, PPARAMOUT:
-                       if n.Name.Addrtaken() {
-                               varwrite = true
+                       if l.Name.Addrtaken() {
+                               memwrite = true
                                continue
                        }
 
-                       if vmatch2(a, n) {
-                               // Direct hit.
+                       if vmatch2(l, r) {
+                               // Direct hit: l appears in r.
                                return true
                        }
                }
        }
 
-       // The variables being written do not appear in n.
-       // However, n might refer to computed addresses
+       // The variables being written do not appear in r.
+       // However, r might refer to computed addresses
        // that are being written.
 
        // If no computed addresses are affected by the writes, no aliasing.
-       if !memwrite && !varwrite {
+       if !memwrite {
                return false
        }
 
-       // If n does not refer to computed addresses
-       // (that is, if n only refers to variables whose addresses
+       // If r does not refer to computed addresses
+       // (that is, if r only refers to variables whose addresses
        // have not been taken), no aliasing.
-       if varexpr(n) {
+       if varexpr(r) {
                return false
        }
 
-       // Otherwise, both the writes and n refer to computed memory addresses.
+       // Otherwise, both the writes and r refer to computed memory addresses.
        // Assume that they might conflict.
        return true
 }