[go] cmd/compile: avoid closure for captured dynamic constants
Cuong Manh Le (Gerrit)
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: avoid closure for captured dynamic constants
Fixes #5370
Change diff
diff --git a/src/cmd/compile/internal/escape/escape.go b/src/cmd/compile/internal/escape/escape.go
index 1c8e382..5dda0e4 100644
--- a/src/cmd/compile/internal/escape/escape.go
+++ b/src/cmd/compile/internal/escape/escape.go
@@ -89,9 +89,10 @@
// A batch holds escape analysis state that's shared across an entire
// batch of functions being analyzed at once.
type batch struct {
- allLocs []*location
- closures []closure
- reassignOracles map[*ir.Func]*ir.ReassignOracle
+ allLocs []*location
+ closures []closure
+ reassignOracles map[*ir.Func]*ir.ReassignOracle
+ constClosureTemps map[*ir.Name]*location // temp -> original closure variable location
heapLoc location
mutatorLoc location
@@ -163,7 +164,6 @@
for _, closure := range b.closures {
b.flowClosure(closure.k, closure.clo)
}
- b.closures = nil
for _, loc := range b.allLocs {
// Try to replace some non-constant expressions with literals.
@@ -176,6 +176,12 @@
}
}
+ // Rewrite constant-capturing closures only after rewriteWithLiterals,
+ // so existing literal/interface rewrites still see the original closure
+ // variables and preserve their diagnostics.
+ b.rewriteConstClosures()
+ b.closures = nil
+
b.walkAll()
b.finish(fns)
}
@@ -371,6 +377,14 @@
}
}
+ for tmp, loc := range b.constClosureTemps {
+ if loc.hasAttr(attrEscapes) {
+ tmp.SetEsc(ir.EscHeap)
+ } else {
+ tmp.SetEsc(ir.EscNone)
+ }
+ }
+
if goexperiment.RuntimeFreegc {
// Look for specific patterns of usage, such as appends
// to slices that we can prove are not aliased.
@@ -409,6 +423,97 @@
escFuncTagged
)
+type constClosureCapturedVar struct {
+ cv *ir.Name
+ lit *ir.BasicLit
+}
+
+// rewriteConstClosures rewrites closures that capture only by-value
+// dynamic constants into ordinary functions.
+//
+// For example:
+//
+// s := ""
+// f := func() { println(s) }
+//
+// can be compiled as a plain function literal with s replaced by an
+// ordinary local initialized from "", avoiding construction of a closure
+// object even if f itself escapes.
+func (b *batch) rewriteConstClosures() {
+ for _, closure := range b.closures {
+ clo := closure.clo
+ fn := clo.Func
+ if !fn.IsClosure() {
+ continue
+ }
+
+ ro := b.reassignOracle(fn)
+ if ro == nil {
+ base.Fatalf("no ReassignOracle for closure %v with parent %v", fn, fn.ClosureParent)
+ }
+
+ var captures []constClosureCapturedVar
+ ok := true
+ for _, cv := range fn.ClosureVars {
+ if !cv.Byval() {
+ ok = false
+ break
+ }
+
+ v := ro.StaticValue(cv.Outer)
+ lit, isLit := v.(*ir.BasicLit)
+ if !isLit || lit.Op() != ir.OLITERAL || !ir.ValidTypeForConst(cv.Type(), lit.Val()) {
+ ok = false
+ break
+ }
+
+ captures = append(captures, constClosureCapturedVar{cv: cv, lit: lit})
+ }
+ if !ok {
+ continue
+ }
+
+ repl := make(map[*ir.Name]*ir.Name)
+ var prefix []ir.Node
+ for _, capture := range captures {
+ cv := capture.cv
+ lit := capture.lit
+ pos := cv.Pos()
+
+ tmp := typecheck.TempAt(pos, fn, cv.Type())
+ repl[cv] = tmp
+ if b.constClosureTemps == nil {
+ b.constClosureTemps = make(map[*ir.Name]*location)
+ }
+ b.constClosureTemps[tmp] = b.oldLoc(cv)
+
+ prefix = append(prefix, typecheck.Stmt(ir.NewDecl(pos, ir.ODCL, tmp)))
+ litVal := ir.NewBasicLit(pos, cv.Type(), lit.Val())
+ prefix = append(prefix, typecheck.Stmt(ir.NewAssignStmt(pos, tmp, litVal)))
+ }
+
+ var edit func(ir.Node) ir.Node
+ edit = func(n ir.Node) ir.Node {
+ if name, ok := n.(*ir.Name); ok && name.Op() == ir.ONAME {
+ if tmp := repl[name]; tmp != nil {
+ return tmp
+ }
+ }
+ ir.EditChildren(n, edit)
+ return n
+ }
+ ir.EditChildren(fn, edit)
+
+ fn.Body.Prepend(prefix...)
+
+ // With all captured variables rewritten to ordinary locals, this is
+ // no longer a closure. walkClosure will lower the OCLOSURE to the
+ // function's ONAME.
+ fn.ClosureVars = nil
+ fn.SetNeedctxt(false)
+ }
+}
+
// Mark labels that have no backjumps to them as not increasing e.loopdepth.
type labelState int
diff --git a/test/escape_dynconst_closure.go b/test/escape_dynconst_closure.go
new file mode 100644
index 0000000..ec684e7
--- /dev/null
+++ b/test/escape_dynconst_closure.go
@@ -0,0 +1,17 @@
+// errorcheck -0 -m -d=closure
+
+// Copyright 2026 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 main
+
+func main() {
+ s := ""
+ f := func() { println(s) } // ERROR "can inline main.func1" "func literal does not escape" "closure converted to global"
+ g(f)
+}
+
+//go:noinline
+func g(func()) {
+}
Change information
- M src/cmd/compile/internal/escape/escape.go
- A test/escape_dynconst_closure.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Message from Cuong Manh Le![]( "Open in Gerrit")
Set Ready For Review
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 4 comments![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.This does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
ro.Init(clofn.ClosureParent)I worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
if lit.Val().Kind() == constant.Int && !constant.Compare(lit.Val(), token.GEQ, constant.MakeInt64(0)) {Why does this matter? Negative constants should be fine.
// Substitute variable references inside the body with literal constantsInstead of replacing the name at every location, maybe we could add a `name := const` assignment at the start of the body instead?
Related details
- Cuong Manh Le
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
ro.Init(clofn.ClosureParent)I worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
I need to read through this a bit more to understand better, but I'll note that I introduced `(*batch).reassignOracle` in escape analysis which tries to avoid repeated work for closure parents:
Maybe that approach is not useful here, but when working on that, I had wondered if the `ir` package should maybe take on making it more convenient to efficiently create ir.ReassignOracles including handling closures well (e.g., maybe a function taking a package and returning a struct that then returns ir.ReassignOracles efficiently for that package, or something like that, though TBH I would need to look at the details again to remember what an actual API might look like).
Something like that might allow deleting `(*batch).reassignOracle`, and maybe could then be used here and elsewhere.
There's also `analyzePreWalk` in `internal/walk.go` which creates ir.ReassignOracles.
While reviewing that, I think Keith might have floated maybe a new field on `ir.Func` or similar if most funcs are going to end up needing an ir.ReassignOracle in practice, though I think some small care would need to be used if so to stop using an ir.ReassignOracle when its no longer valid after many modifications to the IR.
Sorry, probably too off topic for this particular CL, but maybe some food for thought.
Related details
- Cuong Manh Le
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
if lit.Val().Kind() == constant.Int && !constant.Compare(lit.Val(), token.GEQ, constant.MakeInt64(0)) {Why does this matter? Negative constants should be fine.
If we replaced a negative constant to make bultin for example, it will cause compile time panic instead of runtime panic:
```
n := -1
_ = func() { _ = make([]byte, n)}
```
It’s the same reason that escape.(*batch).rewriteWithLiteral also excludes negative ones.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
// Substitute variable references inside the body with literal constantsInstead of replacing the name at every location, maybe we could add a `name := const` assignment at the start of the body instead?
This is indeed my initial implementation, but that causes some liveness issue (IRRC) because I did this transformation during escape analysis. It's probably ok now since this is being done during walk pass.
Further, doing this will also prevent specical case for negative constant above.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Commit-Queue | +1 |
2 comments
ro.Init(clofn.ClosureParent)t hepuddsI worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
I need to read through this a bit more to understand better, but I'll note that I introduced `(*batch).reassignOracle` in escape analysis which tries to avoid repeated work for closure parents:
Maybe that approach is not useful here, but when working on that, I had wondered if the `ir` package should maybe take on making it more convenient to efficiently create ir.ReassignOracles including handling closures well (e.g., maybe a function taking a package and returning a struct that then returns ir.ReassignOracles efficiently for that package, or something like that, though TBH I would need to look at the details again to remember what an actual API might look like).
Something like that might allow deleting `(*batch).reassignOracle`, and maybe could then be used here and elsewhere.
There's also `analyzePreWalk` in `internal/walk.go` which creates ir.ReassignOracles.
While reviewing that, I think Keith might have floated maybe a new field on `ir.Func` or similar if most funcs are going to end up needing an ir.ReassignOracle in practice, though I think some small care would need to be used if so to stop using an ir.ReassignOracle when its no longer valid after many modifications to the IR.
Sorry, probably too off topic for this particular CL, but maybe some food for thought.
I worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
Latest patch adds a cache so we only analyze the parent once. Note that we must walk the parent after it was walked, otherwise, reassignment oracle may not work properly.
// Substitute variable references inside the body with literal constantsCuong Manh LeInstead of replacing the name at every location, maybe we could add a `name := const` assignment at the start of the body instead?
This is indeed my initial implementation, but that causes some liveness issue (IRRC) because I did this transformation during escape analysis. It's probably ok now since this is being done during walk pass.
Further, doing this will also prevent specical case for negative constant above.
Yay, still seeing liveness issue:
```
$ go test -run=Test/unsafe -v cmd/internal/testdir
./regexp/regexp.go:566:43: internal compiler error: '(*Regexp).ReplaceAllLiteralString.func1#0o3auQubr5g=#': value .autotmp_3 (nil) incorrectly live at entry
goroutine 14 [running]:
runtime/debug.Stack()
./runtime/debug/stack.go:26 +0x5e
cmd/compile/internal/base.FatalfAt({0x44155d58?, 0x23e2?}, {0x23e245bc0120, 0x2d}, {0x23e245bb72f0, 0x3, 0x3})
cmd/compile/internal/base/print.go:232 +0x18b
cmd/compile/internal/base.Fatalf(...)
cmd/compile/internal/base/print.go:197
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0x427da9?, {0x44156378?, 0x23e2?}, {0x1095116, 0x27}, {0x23e245bc4020, 0x2, 0xa?})
cmd/compile/internal/ssagen/ssa.go:8017 +0x171
cmd/compile/internal/ssa.(*Func).Fatalf(0x23e245ba8700, {0x1095116, 0x27}, {0x23e245bc4020, 0x2, 0x2})
cmd/compile/internal/ssa/func.go:762 +0x243
cmd/compile/internal/ssagen.(*state).variable(...)
cmd/compile/internal/ssagen/ssa.go:6659
cmd/compile/internal/ssagen.(*state).exprCheckPtr(0x23e245b9aa00, {0x1a820e0, 0x23e243caa820}, 0x1)
cmd/compile/internal/ssagen/ssa.go:3076 +0x6c94
cmd/compile/internal/ssagen.(*state).expr(...)
cmd/compile/internal/ssagen/ssa.go:3026
cmd/compile/internal/ssagen.(*state).exprCheckPtr(0x23e245b9aa00, {0x1a82590, 0x23e243dba0c0}, 0x1)
cmd/compile/internal/ssagen/ssa.go:3579 +0x32eb
cmd/compile/internal/ssagen.(*state).expr(...)
cmd/compile/internal/ssagen/ssa.go:3026
cmd/compile/internal/ssagen.(*state).exprCheckPtr(0x23e245b9aa00, {0x1a82c98, 0x23e243db6600}, 0x1)
cmd/compile/internal/ssagen/ssa.go:3369 +0xfe7
cmd/compile/internal/ssagen.(*state).expr(...)
cmd/compile/internal/ssagen/ssa.go:3026
cmd/compile/internal/ssagen.(*state).stmt(0x23e245b9aa00, {0x1a836c0, 0x23e243db4c80})
cmd/compile/internal/ssagen/ssa.go:1914 +0x69ac
cmd/compile/internal/ssagen.(*state).stmtList(...)
cmd/compile/internal/ssagen/ssa.go:1649
cmd/compile/internal/ssagen.(*state).stmt(0x23e245b9aa00, {0x1a836c0, 0x23e243db4be0})
cmd/compile/internal/ssagen/ssa.go:1674 +0x2a5
cmd/compile/internal/ssagen.(*state).stmtList(...)
cmd/compile/internal/ssagen/ssa.go:1649
cmd/compile/internal/ssagen.buildssa(0x23e243419540, 0x4, 0x0)
cmd/compile/internal/ssagen/ssa.go:574 +0x2bb4
cmd/compile/internal/ssagen.Compile(0x23e243419540, 0x4, 0x0?)
cmd/compile/internal/ssagen/pgen.go:304 +0x88
cmd/compile/internal/gc.compileFunctions.func2()
cmd/compile/internal/gc/compile.go:177 +0x85
created by cmd/compile/internal/gc.compileFunctions in goroutine 1
cmd/compile/internal/gc/compile.go:163 +0xe6
FAIL cmd/internal/testdir [build failed]
FAIL
```
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
ro.Init(clofn.ClosureParent)t hepuddsI worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
Cuong Manh LeI need to read through this a bit more to understand better, but I'll note that I introduced `(*batch).reassignOracle` in escape analysis which tries to avoid repeated work for closure parents:
Maybe that approach is not useful here, but when working on that, I had wondered if the `ir` package should maybe take on making it more convenient to efficiently create ir.ReassignOracles including handling closures well (e.g., maybe a function taking a package and returning a struct that then returns ir.ReassignOracles efficiently for that package, or something like that, though TBH I would need to look at the details again to remember what an actual API might look like).
Something like that might allow deleting `(*batch).reassignOracle`, and maybe could then be used here and elsewhere.
There's also `analyzePreWalk` in `internal/walk.go` which creates ir.ReassignOracles.
While reviewing that, I think Keith might have floated maybe a new field on `ir.Func` or similar if most funcs are going to end up needing an ir.ReassignOracle in practice, though I think some small care would need to be used if so to stop using an ir.ReassignOracle when its no longer valid after many modifications to the IR.
Sorry, probably too off topic for this particular CL, but maybe some food for thought.
I worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
Latest patch adds a cache so we only analyze the parent once. Note that we must walk the parent after it was walked, otherwise, reassignment oracle may not work properly.
Hmm, it seems to me that we have to re-analyze the parent, otherwise nested closure may cause trouble after the immediate enclosing function walked.
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Commit-Queue | +1 |
1 comment
// Substitute variable references inside the body with literal constantsOps, my bad, I missed the code to prepend the tmp declaration into the closure body.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- Cuong Manh Le
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Commit-Queue | +1 |
1 comment
if lit.Val().Kind() == constant.Int && !constant.Compare(lit.Val(), token.GEQ, constant.MakeInt64(0)) {Cuong Manh LeWhy does this matter? Negative constants should be fine.
If we replaced a negative constant to make bultin for example, it will cause compile time panic instead of runtime panic:
```
n := -1
_ = func() { _ = make([]byte, n)}
```
It’s the same reason that escape.(*batch).rewriteWithLiteral also excludes negative ones.
Latest patch use the temp var injection instead of replacing with literals, so this condition could be removed.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.This does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
If something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.Cuong Manh LeThis does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
If something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
It would be nice, if we knew that a captured variable was constant, to not put it in the closure and the copy it back out. Just use that constant in the body.
Makes closures smaller, and code constructing them shorter. And might get some followon constant propagation.
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.Cuong Manh LeThis does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
Keith RandallIf something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
It would be nice, if we knew that a captured variable was constant, to not put it in the closure and the copy it back out. Just use that constant in the body.
Makes closures smaller, and code constructing them shorter. And might get some followon constant propagation.
Fair point.
But at this time, we have already created the temporary for closures (during order pass) using old closure type. So if we optimized the closure here, its type would change and causing a panic since the closure type is tied to its closure variables.
Clearing `clo.Prealloc` might makes things works, but I'm not sure it's better than using already allocated temp.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.Cuong Manh LeThis does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
Keith RandallIf something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
Cuong Manh LeIt would be nice, if we knew that a captured variable was constant, to not put it in the closure and the copy it back out. Just use that constant in the body.
Makes closures smaller, and code constructing them shorter. And might get some followon constant propagation.
Fair point.
But at this time, we have already created the temporary for closures (during order pass) using old closure type. So if we optimized the closure here, its type would change and causing a panic since the closure type is tied to its closure variables.
Clearing `clo.Prealloc` might makes things works, but I'm not sure it's better than using already allocated temp.
Can we do the constant propagation before closure building then? If we got that to work, then this CL would not be needed at all - we'd just naturally convert such closures to the static, noncapturing version.
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
// If not a closure, don't bother wrapping.Cuong Manh LeThis does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
Keith RandallIf something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
Cuong Manh LeIt would be nice, if we knew that a captured variable was constant, to not put it in the closure and the copy it back out. Just use that constant in the body.
Makes closures smaller, and code constructing them shorter. And might get some followon constant propagation.
Keith RandallFair point.
But at this time, we have already created the temporary for closures (during order pass) using old closure type. So if we optimized the closure here, its type would change and causing a panic since the closure type is tied to its closure variables.
Clearing `clo.Prealloc` might makes things works, but I'm not sure it's better than using already allocated temp.
Can we do the constant propagation before closure building then? If we got that to work, then this CL would not be needed at all - we'd just naturally convert such closures to the static, noncapturing version.
Fair point, fixed this in patchset 11, PTAL!
ro.Init(clofn.ClosureParent)t hepuddsI worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
Cuong Manh LeI need to read through this a bit more to understand better, but I'll note that I introduced `(*batch).reassignOracle` in escape analysis which tries to avoid repeated work for closure parents:
Maybe that approach is not useful here, but when working on that, I had wondered if the `ir` package should maybe take on making it more convenient to efficiently create ir.ReassignOracles including handling closures well (e.g., maybe a function taking a package and returning a struct that then returns ir.ReassignOracles efficiently for that package, or something like that, though TBH I would need to look at the details again to remember what an actual API might look like).
Something like that might allow deleting `(*batch).reassignOracle`, and maybe could then be used here and elsewhere.
There's also `analyzePreWalk` in `internal/walk.go` which creates ir.ReassignOracles.
While reviewing that, I think Keith might have floated maybe a new field on `ir.Func` or similar if most funcs are going to end up needing an ir.ReassignOracle in practice, though I think some small care would need to be used if so to stop using an ir.ReassignOracle when its no longer valid after many modifications to the IR.
Sorry, probably too off topic for this particular CL, but maybe some food for thought.
Cuong Manh LeI worry this is going to re-analyze the parent lots of times if the parent has lots of closures in it. Any better way to organize it?
Latest patch adds a cache so we only analyze the parent once. Note that we must walk the parent after it was walked, otherwise, reassignment oracle may not work properly.
Hmm, it seems to me that we have to re-analyze the parent, otherwise nested closure may cause trouble after the immediate enclosing function walked.
Patchset 11 does the optimization during escape analysis, so we won't re-analyze the closure parent a lot of times anymore.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
// If not a closure, don't bother wrapping.Cuong Manh LeThis does feel very all-or-nothing. I think it would be better to constant-propagate any constants into closures, even if it isn't all the closure captured variables.
Keith RandallIf something is present in a closure body, it must be either a local variable, a constant, or a captured variable. I don't there's a case other than captured variables that we need to address.
Since the main motivation here is converting closures into global functions (to avoid the overhead of preparing the closure context), I'm not sure we get a big win if we only partially eliminate the captured variables.
Cuong Manh LeIt would be nice, if we knew that a captured variable was constant, to not put it in the closure and the copy it back out. Just use that constant in the body.
Makes closures smaller, and code constructing them shorter. And might get some followon constant propagation.
Keith RandallFair point.
But at this time, we have already created the temporary for closures (during order pass) using old closure type. So if we optimized the closure here, its type would change and causing a panic since the closure type is tied to its closure variables.
Clearing `clo.Prealloc` might makes things works, but I'm not sure it's better than using already allocated temp.
Cuong Manh LeCan we do the constant propagation before closure building then? If we got that to work, then this CL would not be needed at all - we'd just naturally convert such closures to the static, noncapturing version.
Fair point, fixed this in patchset 11, PTAL!
Hmm, so I fixed the current failed tests in PS 11 locally. However, this kind of code will fail:
```
t.Run("2 pairs", func(t *testing.T) {
s := "abc"
i := 2000
wantAllocs(t, 0, func() {
dl.Info("hello",
"n", i,
"s", s,
)
})
})
```
will now failed because it is re-written as:
```
t.Run("2 pairs", func(t *testing.T) {
s := "abc"
i := 2000
wantAllocs(t, 0, func() {
dl.Info("hello",
"n", i,
"s", s,
)
})
})
```
because it is rewritten as:
```
t.Run("2 pairs", func(t *testing.T) {
wantAllocs(t, 0, func() {
tmp_s := "abc"
tmp_i := 2000
dl.Info("hello",
"n", tmp_i,
"s", tmp_s,
)
})
})
```
Thus it's counted as 2 allocations.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
Hi Cuong, is that maybe an ordering issue?
I wonder if it would help to run your new code after `b.rewriteWithLiterals`? (Or maybe that would create different challenge?)
Related details
- Cuong Manh Le
- Keith Randall
- Keith Randall
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
I think it’s the ordering issue, since both optimizations are not overlapped. It’s just that the new autotmp injections inside the closure causes them visible to `wantAllocs`.
Related details
- Keith Randall
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
Could we re-apply until convergence? It looks like it might move the decls in one level each time.
Related details
- Cuong Manh Le
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
No-Wait-Release
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |