cmd/compile/internal/walk: copy SSA-able variables

order.go ensures expressions that are passed to the runtime by address
are in fact addressable. However, in the case of local variables, if the
variable hasn't already been marked as addrtaken, then taking its
address here will effectively prevent the variable from being converted
to SSA form.

Instead, it's better to just copy the variable into a new temporary,
which we can pass by address instead. This ensures the original variable
can still be converted to SSA form.

Fixes #63332.

Change-Id: I182376d98d419df8bf07c400d84c344c9b82c0fb
Reviewed-on: https://go-review.googlesource.com/c/go/+/541715
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>

diff --git a/src/cmd/compile/internal/walk/order.go b/src/cmd/compile/internal/walk/order.go
index 4d9b2fbee566ef79128cd1c61b4e9ed884b5e6e1..179fbdb99e9872e89f28c28e5401ce3b17b55530 100644 (file)
--- a/src/cmd/compile/internal/walk/order.go
+++ b/src/cmd/compile/internal/walk/order.go
@@ -11,6 +11,7 @@ import (
        "cmd/compile/internal/base"
        "cmd/compile/internal/ir"
        "cmd/compile/internal/reflectdata"
+       "cmd/compile/internal/ssa"
        "cmd/compile/internal/staticinit"
        "cmd/compile/internal/typecheck"
        "cmd/compile/internal/types"
@@ -231,14 +232,29 @@ func (o *orderState) addrTemp(n ir.Node) ir.Node {
                vstat = typecheck.Expr(vstat).(*ir.Name)
                return vstat
        }
+
+       // Prevent taking the address of an SSA-able local variable (#63332).
+       //
+       // TODO(mdempsky): Note that OuterValue unwraps OCONVNOPs, but
+       // IsAddressable does not. It should be possible to skip copying for
+       // at least some of these OCONVNOPs (e.g., reinsert them after the
+       // OADDR operation), but at least walkCompare needs to be fixed to
+       // support that (see trybot failures on go.dev/cl/541715, PS1).
        if ir.IsAddressable(n) {
+               if name, ok := ir.OuterValue(n).(*ir.Name); ok && name.Op() == ir.ONAME {
+                       if name.Class == ir.PAUTO && !name.Addrtaken() && ssa.CanSSA(name.Type()) {
+                               goto Copy
+                       }
+               }
+
                return n
        }
+
+Copy:
        return o.copyExpr(n)
 }
 
 // mapKeyTemp prepares n to be a key in a map runtime call and returns n.
-// It should only be used for map runtime calls which have *_fast* versions.
 // The first parameter is the position of n's containing node, for use in case
 // that n's position is not unique (e.g., if n is an ONAME).
 func (o *orderState) mapKeyTemp(outerPos src.XPos, t *types.Type, n ir.Node) ir.Node {
@@ -603,8 +619,38 @@ func (o *orderState) stmt(n ir.Node) {
        case ir.OAS:
                n := n.(*ir.AssignStmt)
                t := o.markTemp()
+
+               // There's a delicate interaction here between two OINDEXMAP
+               // optimizations.
+               //
+               // First, we want to handle m[k] = append(m[k], ...) with a single
+               // runtime call to mapassign. This requires the m[k] expressions to
+               // satisfy ir.SameSafeExpr in walkAssign.
+               //
+               // But if k is a slow map key type that's passed by reference (e.g.,
+               // byte), then we want to avoid marking user variables as addrtaken,
+               // if that might prevent the compiler from keeping k in a register.
+               //
+               // TODO(mdempsky): It would be better if walk was responsible for
+               // inserting temporaries as needed.
+               mapAppend := n.X.Op() == ir.OINDEXMAP && n.Y.Op() == ir.OAPPEND &&
+                       ir.SameSafeExpr(n.X, n.Y.(*ir.CallExpr).Args[0])
+
                n.X = o.expr(n.X, nil)
-               n.Y = o.expr(n.Y, n.X)
+               if mapAppend {
+                       indexLHS := n.X.(*ir.IndexExpr)
+                       indexLHS.X = o.cheapExpr(indexLHS.X)
+                       indexLHS.Index = o.cheapExpr(indexLHS.Index)
+
+                       call := n.Y.(*ir.CallExpr)
+                       indexRHS := call.Args[0].(*ir.IndexExpr)
+                       indexRHS.X = indexLHS.X
+                       indexRHS.Index = indexLHS.Index
+
+                       o.exprList(call.Args[1:])
+               } else {
+                       n.Y = o.expr(n.Y, n.X)
+               }
                o.mapAssign(n)
                o.popTemp(t)
 
@@ -1158,7 +1204,7 @@ func (o *orderState) expr1(n, lhs ir.Node) ir.Node {
                        }
                }
 
-               // key must be addressable
+               // key may need to be be addressable
                n.Index = o.mapKeyTemp(n.Pos(), n.X.Type(), n.Index)
                if needCopy {
                        return o.copyExpr(n)

diff --git a/test/codegen/issue63332.go b/test/codegen/issue63332.go
new file mode 100644 (file)
index 0000000..dbe671d
--- /dev/null
+++ b/test/codegen/issue63332.go
@@ -0,0 +1,14 @@
+// asmcheck
+
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package codegen
+
+func issue63332(c chan int) {
+       x := 0
+       // amd64:-`MOVQ`
+       x += 2
+       c <- x
+}