From 00715d089d68c1dd43ed1f508e8937c5208fb6f0 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Fri, 10 Nov 2023 18:03:00 -0800 Subject: [PATCH] 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 Reviewed-by: Keith Randall Reviewed-by: Keith Randall Auto-Submit: Matthew Dempsky --- src/cmd/compile/internal/walk/order.go | 52 ++++++++++++++++++++++++-- test/codegen/issue63332.go | 14 +++++++ 2 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 test/codegen/issue63332.go diff --git a/src/cmd/compile/internal/walk/order.go b/src/cmd/compile/internal/walk/order.go index 4d9b2fbee5..179fbdb99e 100644 --- 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 index 0000000000..dbe671d247 --- /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 +} -- 2.48.1