[go] cmd/compile: change testing.B.Loop keep alive semantic

[Junyang Shao (Gerrit)]

Junyang Shao has uploaded the change for review

Commit message

cmd/compile: change testing.B.Loop keep alive semantic

WIP

This CL implements this initial design of testing.B.Loop's keep variable
alive semantic:
https://github.com/golang/go/issues/61515#issuecomment-2407963248.

Fixes #73137.

Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Change diff

diff --git a/src/cmd/compile/internal/bloop/bloop.go b/src/cmd/compile/internal/bloop/bloop.go
new file mode 100644
index 0000000..65105be
--- /dev/null
+++ b/src/cmd/compile/internal/bloop/bloop.go
@@ -0,0 +1,141 @@
+// Copyright 2022 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 bloop
+
+import (
+	"cmd/compile/internal/ir"
+)
+
+// This package contains support routines for keeping
+// statements alive
+// in such loops (example):
+//
+//	b.Run(name, func(b *testing.B) {
+//		for b.Loop() {
+//			var a, b int
+//			a = 5
+//			b = 6
+//			f(a, b)
+//		}
+//	}
+//
+// The results of a, b and f(a, b) will be kept alive.
+//
+// Formally, the lhs(if they are [ir.Name]-s) of
+// [ir.AssignStmt], [ir.AssignListStmt],
+// [ir.AssignOpStmt]; the results of [ir.CallExpr] and
+// the [X] field of [ir.Decl] will be kept alive.
+//
+// The keepalive logic is implemented as a new boolean
+// field in these affected nodes, and this field will
+// be checked at ssagen and an [OpVarLive] will be
+// inserted for corresponding symbols of the marked
+// nodes.
+
+func markStmt(stmt ir.Node) {
+	switch n := stmt.(type) {
+	case *ir.AssignStmt:
+		n.KeepXNameAlive = true
+	case *ir.AssignListStmt:
+		n.KeepLhsNamesAlive = true
+	case *ir.AssignOpStmt:
+		n.KeepXNameAlive = true
+	case *ir.Decl:
+		n.KeepXAlive = true
+	case *ir.CallExpr:
+		n.KeepResultsAlive = true
+	}
+}
+
+// isTestingBLoop returns true if it matches the node as a
+// testing.(*B).Loop. See issue #61515.
+func isTestingBLoop(t ir.Node) bool {
+	if t.Op() != ir.OFOR {
+		return false
+	}
+	nFor, ok := t.(*ir.ForStmt)
+	if !ok || nFor.Cond == nil || nFor.Cond.Op() != ir.OCALLFUNC {
+		return false
+	}
+	n, ok := nFor.Cond.(*ir.CallExpr)
+	if !ok || n.Fun == nil || n.Fun.Op() != ir.OMETHEXPR {
+		return false
+	}
+	name := ir.MethodExprName(n.Fun)
+	if name == nil {
+		return false
+	}
+	if fSym := name.Sym(); fSym != nil && name.Class == ir.PFUNC && fSym.Pkg != nil &&
+		fSym.Name == "(*B).Loop" && fSym.Pkg.Path == "testing" {
+		// Attempting to match a function call to testing.(*B).Loop
+		return true
+	}
+	return false
+}
+
+// BloopWalk performs a walk on all functions in the package
+// if it imports testing and mark all qualified statements in a
+//
+//	for b.Loop() {...}
+//
+// loop's body.
+func BloopWalk(pkg *ir.Package) {
+	for _, i := range pkg.Imports {
+		if i.Path == "testing" {
+			return
+		}
+	}
+	for _, fn := range pkg.Funcs {
+		ir.Visit(fn, func(n ir.Node) {
+			if !isTestingBLoop(n) {
+				return
+			}
+			body := n.(*ir.ForStmt).Body
+			for _, stmt := range body {
+				markStmt(stmt)
+			}
+		})
+	}
+}
+
+// checkMarkedStmts checks all marked statements for the node.
+// It will return true if the node is marked and its interested
+// args are of the right shape, i.e:
+// for assign, assign list and assign op stmt, it will check if the
+// lhs are all namess.
+// for decl and call, their lhs are guaranteed to be names.
+func CheckMarkedStmts(stmt ir.Node) bool {
+	switch n := stmt.(type) {
+	case *ir.AssignStmt:
+		if n.KeepXNameAlive {
+			return n.X.Op() == ir.ONAME
+		}
+	case *ir.AssignListStmt:
+		if n.KeepLhsNamesAlive {
+			for _, lhs := range n.Lhs {
+				if lhs.Op() != ir.ONAME {
+					return false
+				}
+			}
+			return true
+		}
+	// In practice, this is not seen in ssagen because it is desugared away.
+	case *ir.AssignOpStmt:
+		if n.KeepXNameAlive {
+			return n.X.Op() == ir.ONAME
+		}
+	case *ir.Decl:
+		return n.KeepXAlive
+	case *ir.CallExpr:
+		// Results are guaranteed to be a name.
+		for _, res := range n.Fun.Type().Results() {
+			if _, ok := res.Nname.(*ir.Name); !ok {
+				return false
+			}
+		}
+		return n.KeepResultsAlive
+	}
+	return false
+}
diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go
index 42e2afa..5bb6630 100644
--- a/src/cmd/compile/internal/gc/main.go
+++ b/src/cmd/compile/internal/gc/main.go
@@ -8,6 +8,7 @@
 	"bufio"
 	"bytes"
 	"cmd/compile/internal/base"
+	"cmd/compile/internal/bloop"
 	"cmd/compile/internal/coverage"
 	"cmd/compile/internal/deadlocals"
 	"cmd/compile/internal/dwarfgen"
@@ -222,6 +223,9 @@
 	// Apply coverage fixups, if applicable.
 	coverage.Fixup()
 
+	// Apply bloop markings if it imports the testing package
+	bloop.BloopWalk(typecheck.Target)
+
 	// Read profile file and build profile-graph and weighted-call-graph.
 	base.Timer.Start("fe", "pgo-load-profile")
 	var profile *pgoir.Profile
diff --git a/src/cmd/compile/internal/inline/interleaved/interleaved.go b/src/cmd/compile/internal/inline/interleaved/interleaved.go
index c83bbdb..80a0cb9 100644
--- a/src/cmd/compile/internal/inline/interleaved/interleaved.go
+++ b/src/cmd/compile/internal/inline/interleaved/interleaved.go
@@ -254,28 +254,6 @@
 		return n // already visited n.X before wrapping
 	}
 
-	if isTestingBLoop(n) {
-		// No inlining nor devirtualization performed on b.Loop body
-		if base.Flag.LowerM > 0 {
-			fmt.Printf("%v: skip inlining within testing.B.loop for %v\n", ir.Line(n), n)
-		}
-		// We still want to explore inlining opportunities in other parts of ForStmt.
-		nFor, _ := n.(*ir.ForStmt)
-		nForInit := nFor.Init()
-		for i, x := range nForInit {
-			if x != nil {
-				nForInit[i] = s.mark(x)
-			}
-		}
-		if nFor.Cond != nil {
-			nFor.Cond = s.mark(nFor.Cond)
-		}
-		if nFor.Post != nil {
-			nFor.Post = s.mark(nFor.Post)
-		}
-		return n
-	}
-
 	if p != nil {
 		n = p.X // in this case p was copied in from a (marked) inlined function, this is a new unvisited node.
 	}
@@ -371,29 +349,3 @@
 	}
 	return false
 }
-
-// isTestingBLoop returns true if it matches the node as a
-// testing.(*B).Loop. See issue #61515.
-func isTestingBLoop(t ir.Node) bool {
-	if t.Op() != ir.OFOR {
-		return false
-	}
-	nFor, ok := t.(*ir.ForStmt)
-	if !ok || nFor.Cond == nil || nFor.Cond.Op() != ir.OCALLFUNC {
-		return false
-	}
-	n, ok := nFor.Cond.(*ir.CallExpr)
-	if !ok || n.Fun == nil || n.Fun.Op() != ir.OMETHEXPR {
-		return false
-	}
-	name := ir.MethodExprName(n.Fun)
-	if name == nil {
-		return false
-	}
-	if fSym := name.Sym(); fSym != nil && name.Class == ir.PFUNC && fSym.Pkg != nil &&
-		fSym.Name == "(*B).Loop" && fSym.Pkg.Path == "testing" {
-		// Attempting to match a function call to testing.(*B).Loop
-		return true
-	}
-	return false
-}
diff --git a/src/cmd/compile/internal/ir/expr.go b/src/cmd/compile/internal/ir/expr.go
index 7a75ff4..560b90f 100644
--- a/src/cmd/compile/internal/ir/expr.go
+++ b/src/cmd/compile/internal/ir/expr.go
@@ -184,14 +184,15 @@
 // A CallExpr is a function call Fun(Args).
 type CallExpr struct {
 	miniExpr
-	Fun       Node
-	Args      Nodes
-	DeferAt   Node
-	RType     Node    `mknode:"-"` // see reflectdata/helpers.go
-	KeepAlive []*Name // vars to be kept alive until call returns
-	IsDDD     bool
-	GoDefer   bool // whether this call is part of a go or defer statement
-	NoInline  bool // whether this call must not be inlined
+	Fun              Node
+	Args             Nodes
+	DeferAt          Node
+	RType            Node    `mknode:"-"` // see reflectdata/helpers.go
+	KeepAlive        []*Name // vars to be kept alive until call returns
+	IsDDD            bool
+	GoDefer          bool // whether this call is part of a go or defer statement
+	NoInline         bool // whether this call must not be inlined
+	KeepResultsAlive bool
 }
 
 func NewCallExpr(pos src.XPos, op Op, fun Node, args []Node) *CallExpr {
diff --git a/src/cmd/compile/internal/ir/stmt.go b/src/cmd/compile/internal/ir/stmt.go
index 0801ecdd..5350b1a 100644
--- a/src/cmd/compile/internal/ir/stmt.go
+++ b/src/cmd/compile/internal/ir/stmt.go
@@ -15,7 +15,8 @@
 // A Decl is a declaration of a const, type, or var. (A declared func is a Func.)
 type Decl struct {
 	miniNode
-	X *Name // the thing being declared
+	X          *Name // the thing being declared
+	KeepXAlive bool  // X should be kept alive.
 }
 
 func NewDecl(pos src.XPos, op Op, x *Name) *Decl {
@@ -61,9 +62,10 @@
 // If Def is true, the assignment is a :=.
 type AssignListStmt struct {
 	miniStmt
-	Lhs Nodes
-	Def bool
-	Rhs Nodes
+	Lhs               Nodes
+	Def               bool
+	Rhs               Nodes
+	KeepLhsNamesAlive bool
 }
 
 func NewAssignListStmt(pos src.XPos, op Op, lhs, rhs []Node) *AssignListStmt {
@@ -88,9 +90,10 @@
 // If Def is true, the assignment is a :=.
 type AssignStmt struct {
 	miniStmt
-	X   Node
-	Def bool
-	Y   Node
+	X              Node
+	Def            bool
+	Y              Node
+	KeepXNameAlive bool
 }
 
 func NewAssignStmt(pos src.XPos, x, y Node) *AssignStmt {
@@ -112,10 +115,11 @@
 // An AssignOpStmt is an AsOp= assignment statement: X AsOp= Y.
 type AssignOpStmt struct {
 	miniStmt
-	X      Node
-	AsOp   Op // OADD etc
-	Y      Node
-	IncDec bool // actually ++ or --
+	X              Node
+	AsOp           Op // OADD etc
+	Y              Node
+	IncDec         bool // actually ++ or --
+	KeepXNameAlive bool
 }
 
 func NewAssignOpStmt(pos src.XPos, asOp Op, x, y Node) *AssignOpStmt {
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index ae7d575..0bf56e1 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -20,6 +20,7 @@
 
 	"cmd/compile/internal/abi"
 	"cmd/compile/internal/base"
+	"cmd/compile/internal/bloop"
 	"cmd/compile/internal/ir"
 	"cmd/compile/internal/liveness"
 	"cmd/compile/internal/objw"
@@ -1642,6 +1643,11 @@
 		n := n.(*ir.CallExpr)
 		if ir.IsIntrinsicCall(n) {
 			s.intrinsicCall(n)
+			if bloop.CheckMarkedStmts(n) {
+				for _, res := range n.Fun.Type().Results() {
+					s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, res.Nname.(*ir.Name), s.mem(), false)
+				}
+			}
 			return
 		}
 		fallthrough
@@ -1665,6 +1671,11 @@
 				// go through SSA.
 			}
 		}
+		if bloop.CheckMarkedStmts(n) {
+			for _, res := range n.Fun.Type().Results() {
+				s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, res.Nname.(*ir.Name), s.mem(), false)
+			}
+		}
 	case ir.ODEFER:
 		n := n.(*ir.GoDeferStmt)
 		if base.Debug.Defer > 0 {
@@ -1713,6 +1724,10 @@
 		}
 		s.assign(n.Lhs[0], res, deref, 0)
 		s.assign(n.Lhs[1], resok, false, 0)
+		if bloop.CheckMarkedStmts(n) {
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.Lhs[0].Name(), s.mem(), false)
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.Lhs[1].Name(), s.mem(), false)
+		}
 		return
 
 	case ir.OAS2FUNC:
@@ -1727,6 +1742,10 @@
 		v2 := s.newValue1(ssa.OpSelect1, n.Lhs[1].Type(), v)
 		s.assign(n.Lhs[0], v1, false, 0)
 		s.assign(n.Lhs[1], v2, false, 0)
+		if bloop.CheckMarkedStmts(n) {
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.Lhs[0].Name(), s.mem(), false)
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.Lhs[1].Name(), s.mem(), false)
+		}
 		return
 
 	case ir.ODCL:
@@ -1734,6 +1753,9 @@
 		if v := n.X; v.Esc() == ir.EscHeap {
 			s.newHeapaddr(v)
 		}
+		if bloop.CheckMarkedStmts(n) {
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.X, s.mem(), false)
+		}
 
 	case ir.OLABEL:
 		n := n.(*ir.LabelStmt)
@@ -1906,6 +1928,9 @@
 		}
 
 		s.assignWhichMayOverlap(n.X, r, deref, skip, mayOverlap)
+		if bloop.CheckMarkedStmts(n) {
+			s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, n.X.Name(), s.mem(), false)
+		}
 
 	case ir.OIF:
 		n := n.(*ir.IfStmt)
@@ -3654,13 +3679,24 @@
 	case ir.OCALLFUNC:
 		n := n.(*ir.CallExpr)
 		if ir.IsIntrinsicCall(n) {
+			if bloop.CheckMarkedStmts(n) {
+				for _, res := range n.Fun.Type().Results() {
+					s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, res.Nname.(*ir.Name), s.mem(), false)
+				}
+			}
 			return s.intrinsicCall(n)
 		}
 		fallthrough
 
 	case ir.OCALLINTER:
 		n := n.(*ir.CallExpr)
-		return s.callResult(n, callNormal)
+		call := s.callResult(n, callNormal)
+		if bloop.CheckMarkedStmts(n) {
+			for _, res := range n.Fun.Type().Results() {
+				s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, res.Nname.(*ir.Name), s.mem(), false)
+			}
+		}
+		return call
 
 	case ir.OGETG:
 		n := n.(*ir.CallExpr)
@@ -5077,7 +5113,13 @@
 		return s.newValue1(ssa.OpCopy, t, addr) // ensure that addr has the right type
 	case ir.OCALLFUNC, ir.OCALLINTER:
 		n := n.(*ir.CallExpr)
-		return s.callAddr(n, callNormal)
+		call := s.callAddr(n, callNormal)
+		if bloop.CheckMarkedStmts(n) {
+			for _, res := range n.Fun.Type().Results() {
+				s.vars[memVar] = s.newValue1Apos(ssa.OpVarLive, types.TypeMem, res.Nname.(*ir.Name), s.mem(), false)
+			}
+		}
+		return call
 	case ir.ODOTTYPE, ir.ODYNAMICDOTTYPE:
 		var v *ssa.Value
 		if n.Op() == ir.ODOTTYPE {
diff --git a/src/cmd/compile/internal/walk/assign.go b/src/cmd/compile/internal/walk/assign.go
index 63b6a1d..ccec81a 100644
--- a/src/cmd/compile/internal/walk/assign.go
+++ b/src/cmd/compile/internal/walk/assign.go
@@ -49,7 +49,9 @@
 
 	if n.Op() == ir.OASOP {
 		// Rewrite x op= y into x = x op y.
+		keepXAlive := n.(*ir.AssignOpStmt).KeepXNameAlive
 		n = ir.NewAssignStmt(base.Pos, left, typecheck.Expr(ir.NewBinaryExpr(base.Pos, n.(*ir.AssignOpStmt).AsOp, left, right)))
+		n.(*ir.AssignStmt).KeepXNameAlive = keepXAlive
 	} else {
 		n.(*ir.AssignStmt).X = left
 	}

Change information

Files:

• A src/cmd/compile/internal/bloop/bloop.go
• M src/cmd/compile/internal/gc/main.go
• M src/cmd/compile/internal/inline/interleaved/interleaved.go
• M src/cmd/compile/internal/ir/expr.go
• M src/cmd/compile/internal/ir/stmt.go
• M src/cmd/compile/internal/ssagen/ssa.go
• M src/cmd/compile/internal/walk/assign.go

Change size: L

Delta: 7 files changed, 215 insertions(+), 69 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 1

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and David Chase

Junyang Shao voted

Code-Review	+1
Commit-Queue	+1

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• David Chase

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 10

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: David Chase <drc...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Mon, 03 Nov 2025 23:14:41 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and David Chase

Junyang Shao voted and added 1 comment

Votes added by Junyang Shao

Code-Review

+0

1 comment

File src/cmd/compile/internal/ssagen/intrinsics.go

Line 161, Patchset 10 (Latest): add("runtime", "varlive",
func(s *state, n *ir.CallExpr, args []*ssa.Value) *ssa.Value {
data := s.newValue1(ssa.OpIData, s.f.Config.Types.BytePtr, args[0])
s.vars[memVar] = s.newValue2(ssa.OpKeepAlive, types.TypeMem, data, s.mem())
return nil
},
all...)

Junyang Shao . resolved

Right now `varlive` is just lowered to `KeepAlive`, because `KeepAlive` takes a pointer but `VarLive` requires a symbol. I tried some hard-coded rules to find the symbol, but there often exist some other cases that break the assumption.

It looks like `KeepAlive` has the same position as `VarLive` for liveness, and it prevents deadstore and deadcode, that looks enough for now?

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• David Chase

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 10

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-CC: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: David Chase <drc...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Mon, 03 Nov 2025 23:26:19 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and David Chase

Junyang Shao added 1 comment

File src/cmd/compile/internal/bloop/bloop.go

Line 36, Patchset 10 (Latest):// The keepalive logic is implemented as a new boolean


// field in these affected nodes, and this field will


// be checked at ssagen and an [OpVarLive] will be


// inserted for corresponding symbols of the marked


// nodes.

Junyang Shao . unresolved

This is stale and will update

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• David Chase

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 10

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: David Chase <drc...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Mon, 03 Nov 2025 23:29:00 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[David Chase (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and Junyang Shao

David Chase added 5 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

David Chase . resolved

LGTM, the comments need addressing but they should be easy.

I think sync'ing with tip (rebasing on tip) should clear the boring crypto failure, if not, nudge the limit parameter up slightly on the test case (cmd/compile/internal/ssa/stmtlines_test.go:143).

File src/cmd/compile/internal/bloop/bloop.go

Line 66, Patchset 10 (Latest):// varliveAt generates a runtime.varlive CallExpr and sets the magic fields
// at pos, if successful it returns a Block that puts curNode at the beginning.

David Chase . unresolved

this part did not help me much. It looks like what is happening is

varliveAt returns a statement that is either curNode, or a block containing curNode followed by a call to runtime.varlive for each ONAME in ns. These calls ensure that names in ns will be live until after curNode's execution.

Line 105, Patchset 10 (Latest):// markStmt marks stmts that have some results
// that need to be kept alive, and returns the marked
// stmt.
func markStmt(curFn *ir.Func, stmt ir.Node) (ret ir.Node) {

David Chase . unresolved

maybe call this "preserveStmt"? There might be a better name than that, but this feels "markStmt" is left over from when it tried a different way of keeping code alive.

So,
// preserveStmt transforms stmt so that any names defined/assigned within it
// are used after stmt's execution, preventing their dead code elimination.
// The return value is the transformed statement.

Line 213, Patchset 10 (Latest): var isInBloop bool

David Chase . unresolved

I think this probably ought to be a counter. It seems like a really bad idea to nest b.Loop within another b.Loop, but it is certainly not statically forbidden and might only give weird results. So instead of a boolean isInBloop, a counter bLoopNesting, increment, decrement, and compare with zero.

File src/runtime/bloop.go

Line 10, Patchset 10 (Latest):func varlive(interface{})

David Chase . unresolved

Just to be clear, preceding this with //go:noescape was not good enough to get rid of the escape, so the special case in escape analysis is necessary? (Other people besides me will ask this same question, so it's good to have it in the review.)

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• Junyang Shao

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 10

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Tue, 04 Nov 2025 15:47:40 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[David Chase (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and Junyang Shao

David Chase voted Code-Review+2

Code-Review

+2

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• Junyang Shao

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 10

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Tue, 04 Nov 2025 15:47:49 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and Junyang Shao

Junyang Shao uploaded new patchset

Junyang Shao uploaded patch set #11 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• Junyang Shao

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 11

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements, Cherry Mui and Junyang Shao

Junyang Shao uploaded new patchset

Junyang Shao uploaded patch set #12 to this change.

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui
• Junyang Shao

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 12

[Junyang Shao (Gerrit)]

Attention needed from Austin Clements and Cherry Mui

Junyang Shao voted and added 6 comments

Votes added by Junyang Shao

Commit-Queue

+1

6 comments

File src/cmd/compile/internal/bloop/bloop.go

Line 36, Patchset 10:// The keepalive logic is implemented as a new boolean


// field in these affected nodes, and this field will
// be checked at ssagen and an [OpVarLive] will be
// inserted for corresponding symbols of the marked
// nodes.

Junyang Shao . resolved

This is stale and will update

Junyang Shao

Done

Line 66, Patchset 10:// varliveAt generates a runtime.varlive CallExpr and sets the magic fields


// at pos, if successful it returns a Block that puts curNode at the beginning.

David Chase . resolved

this part did not help me much. It looks like what is happening is

varliveAt returns a statement that is either curNode, or a block containing curNode followed by a call to runtime.varlive for each ONAME in ns. These calls ensure that names in ns will be live until after curNode's execution.

Junyang Shao

Done

Line 105, Patchset 10:// markStmt marks stmts that have some results


// that need to be kept alive, and returns the marked
// stmt.
func markStmt(curFn *ir.Func, stmt ir.Node) (ret ir.Node) {

David Chase . resolved

maybe call this "preserveStmt"? There might be a better name than that, but this feels "markStmt" is left over from when it tried a different way of keeping code alive.

So,
// preserveStmt transforms stmt so that any names defined/assigned within it
// are used after stmt's execution, preventing their dead code elimination.
// The return value is the transformed statement.

Junyang Shao

Done

Line 213, Patchset 10: var isInBloop bool

David Chase . resolved

I think this probably ought to be a counter. It seems like a really bad idea to nest b.Loop within another b.Loop, but it is certainly not statically forbidden and might only give weird results. So instead of a boolean isInBloop, a counter bLoopNesting, increment, decrement, and compare with zero.

Junyang Shao

Yeah that's a possibility, thanks for noticing this.
Updated

File src/cmd/compile/internal/ssagen/intrinsics.go

Line 161, Patchset 10: add("runtime", "varlive",


func(s *state, n *ir.CallExpr, args []*ssa.Value) *ssa.Value {
data := s.newValue1(ssa.OpIData, s.f.Config.Types.BytePtr, args[0])
s.vars[memVar] = s.newValue2(ssa.OpKeepAlive, types.TypeMem, data, s.mem())
return nil
},
all...)

Junyang Shao . resolved

Right now `varlive` is just lowered to `KeepAlive`, because `KeepAlive` takes a pointer but `VarLive` requires a symbol. I tried some hard-coded rules to find the symbol, but there often exist some other cases that break the assumption.

It looks like `KeepAlive` has the same position as `VarLive` for liveness, and it prevents deadstore and deadcode, that looks enough for now?

Junyang Shao

I added a comment there too.

File src/runtime/bloop.go

Line 10, Patchset 10:func varlive(interface{})

David Chase . resolved

Just to be clear, preceding this with //go:noescape was not good enough to get rid of the escape, so the special case in escape analysis is necessary? (Other people besides me will ask this same question, so it's good to have it in the review.)

Junyang Shao

Yes, I tried //go:noescape here and it has no effect on escape analysis, the args still escaped.

I assume it's because when generated by the compiler and being analyzed by escape, this symbol does not come from here but from the generated code in builtin.go. I didn't try adding the directive in builtin because the generator will ignore comments anyway, either I change the builtin generator(and I am still not sure if that will have effect or not), or I change the compiler(which will certainly have effect). :D

Open in Gerrit

Related details

Attention is currently required from:

• Austin Clements
• Cherry Mui

Submit Requirements:

• Code-Review
• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I8060470dbcb0dda0819334f3615cc391ff0f6501

Gerrit-Change-Number: 716660

Gerrit-PatchSet: 12

Gerrit-Owner: Junyang Shao <shaoj...@google.com>

Gerrit-Reviewer: Austin Clements <aus...@google.com>

Gerrit-Reviewer: Cherry Mui <cher...@google.com>

Gerrit-Reviewer: David Chase <drc...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Junyang Shao <shaoj...@google.com>

Gerrit-Attention: Cherry Mui <cher...@google.com>

Gerrit-Attention: Austin Clements <aus...@google.com>

Gerrit-Comment-Date: Tue, 04 Nov 2025 17:31:08 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: David Chase <drc...@google.com>

Comment-In-Reply-To: Junyang Shao <shaoj...@google.com>