[go] cmd/compile/internal/ssa: more aggressive on dead auto elim
33 views
Skip to first unread message
Youlin Feng (Gerrit)
Sep 11, 2025, 12:03:28 PMSep 11
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Youlin Feng has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile/internal/ssa: more aggressive on dead auto elim
Updates #75398
Change-Id: If75e9caaa50d460efc31a94565b9ba28c8158771
Change diff
diff --git a/src/cmd/compile/internal/ir/node.go b/src/cmd/compile/internal/ir/node.go
index 003ec15..151e699 100644
--- a/src/cmd/compile/internal/ir/node.go
+++ b/src/cmd/compile/internal/ir/node.go
@@ -426,6 +426,14 @@
(*s)[n] = struct{}{}
}
+// Del deletes n from s.
+func (s *NameSet) Del(n *Name) {
+ if s == nil {
+ return
+ }
+ delete(*s, n)
+}
+
type PragmaFlag uint16
const (
diff --git a/src/cmd/compile/internal/ssa/deadstore.go b/src/cmd/compile/internal/ssa/deadstore.go
index 9e67e83..a2a7766 100644
--- a/src/cmd/compile/internal/ssa/deadstore.go
+++ b/src/cmd/compile/internal/ssa/deadstore.go
@@ -202,9 +202,10 @@
// reaches stores then we delete all the stores. The other operations will then
// be eliminated by the dead code elimination pass.
func elimDeadAutosGeneric(f *Func) {
- addr := make(map[*Value]*ir.Name) // values that the address of the auto reaches
- elim := make(map[*Value]*ir.Name) // values that could be eliminated if the auto is
- var used ir.NameSet // used autos that must be kept
+ addr := make(map[*Value]*ir.Name) // values that the address of the auto reaches
+ elim := make(map[*Value]*ir.Name) // values that could be eliminated if the auto is
+ rely := make(map[*ir.Name]*ir.Name) // keys rely on values to be alived
+ var used ir.NameSet // used autos that must be kept
// visit the value and report whether any of the maps are updated
visit := func(v *Value) (changed bool) {
@@ -247,6 +248,7 @@
used.Add(n)
changed = true
}
+ rely[n] = nil
return
case OpStore, OpMove, OpZero:
// v should be eliminated if we eliminate the auto.
@@ -282,6 +284,15 @@
used.Add(n)
changed = true
}
+ var name *ir.Name
+ if nm, ok := elim[v]; ok && v.Op == OpMove {
+ name = nm
+ }
+ if o, ok := rely[n]; ok && o != name {
+ rely[n] = nil
+ } else if !ok {
+ rely[n] = name
+ }
}
}
return
@@ -302,6 +313,7 @@
used.Add(n)
changed = true
}
+ rely[n] = nil
}
}
if node == nil {
@@ -346,6 +358,18 @@
}
}
+ for range 4 {
+ changed := false
+ for n, o := range rely {
+ if o != nil && used.Has(n) && !used.Has(o) {
+ used.Del(n)
+ changed = true
+ }
+ }
+ if !changed {
+ break
+ }
+ }
// Eliminate stores to unread autos.
for v, n := range elim {
if used.Has(n) {
Change information
Files:
- M src/cmd/compile/internal/ir/node.go
- M src/cmd/compile/internal/ssa/deadstore.go
Change size: S
Delta: 2 files changed, 35 insertions(+), 3 deletions(-)
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. |
Youlin Feng (Gerrit)
Sep 11, 2025, 9:09:23 PMSep 11
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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. |
Youlin Feng (Gerrit)
Sep 11, 2025, 9:26:41 PMSep 11
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #3 to this change.
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 11, 2025, 11:24:13 PMSep 11
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #4 to this change.
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 11, 2025, 11:35:59 PMSep 11
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #5 to this change.
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 12, 2025, 10:18:33 AMSep 12
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #6 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 12, 2025, 10:19:48 AMSep 12
to goph...@pubsubhelper.golang.org, Go LUCI, Robert Griesemer, Keith Randall, Martin Möhrmann, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 23, 2025, 8:39:10 AMSep 23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #7 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 23, 2025, 9:31:55 AMSep 23
to goph...@pubsubhelper.golang.org, Go LUCI, Robert Griesemer, Keith Randall, Martin Möhrmann, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 24, 2025, 9:26:16 PMSep 24
to goph...@pubsubhelper.golang.org, Go LUCI, Robert Griesemer, Keith Randall, Martin Möhrmann, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Keith Randall, Martin Möhrmann and Robert Griesemer
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Keith Randall
- Martin Möhrmann
- Robert Griesemer
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. |
Youlin Feng (Gerrit)
Sep 25, 2025, 9:04:11 AMSep 25
to goph...@pubsubhelper.golang.org, David Chase, Go LUCI, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Sep 27, 2025, 4:47:29 PMSep 27
to Youlin Feng, goph...@pubsubhelper.golang.org, David Chase, Go LUCI, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall added 4 comments![]( "Open in Gerrit")
Patchset-level comments
Keith Randall . resolved
Interesting idea. I'm not entirely sure why this is safe. I think it needs more comments as to how `rely` and `elim` interact, and how we're only throwing away autos that we are sure are never used.
File src/cmd/compile/internal/ir/node.go
File src/cmd/compile/internal/ssa/deadstore.go
Line 207, Patchset 8 (Latest):
rely := make(map[*ir.Name]*ir.Name) // keys rely on values to be keptKeith Randall . unresolved
I don't understand this comment. Maybe make it multiline and put a more detailed description?
rely[x] = y if a use of x depends on y being available.
rely[x] = nil means x must be available.
Line 295, Patchset 8 (Latest):
// Relying on just one name, or setting value to nil,Keith Randall . unresolved
Does this not depend on copy direction? It seems this code is the same whether n is from v.Args[0] (the destination) or v.Args[1] (the source).
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Sep 27, 2025, 10:23:22 PMSep 27
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #9 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Sep 27, 2025, 10:27:20 PMSep 27
to goph...@pubsubhelper.golang.org, David Chase, Go LUCI, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted and added 4 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
4 comments
File src/cmd/compile/internal/ir/node.go
Line 429, Patchset 8:
// Del deletes n from s.Keith Randall . resolved
Youlin Fengif it is present
Done
File src/cmd/compile/internal/ssa/deadstore.go
Line 207, Patchset 8:
rely := make(map[*ir.Name]*ir.Name) // keys rely on values to be keptKeith Randall . resolved
I don't understand this comment. Maybe make it multiline and put a more detailed description?
rely[x] = y if a use of x depends on y being available.
rely[x] = nil means x must be available.
Youlin Feng
Done
Line 262, Patchset 8:
args = args[1:]Youlin Feng . resolved
Here we have a slice operation that removes the destination arg.
Does this not depend on copy direction? It seems this code is the same whether n is from v.Args[0] (the destination) or v.Args[1] (the source).
Youlin Feng
In the switch statement above, we have a slice operation on args that removes the destination arg. Since we are only interested in OpMove, maybe this would be ok?
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Sep 28, 2025, 4:31:23 AMSep 28
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Keith Randall . resolved
Interesting idea. I'm not entirely sure why this is safe. I think it needs more comments as to how `rely` and `elim` interact, and how we're only throwing away autos that we are sure are never used.
Youlin Feng
Before this CL, `elimDeadAutosGeneric` already can eliminate the autos that are never readed, I mean it can make sure which auto is never readed, so we can use the same method later to ensure that a auto is only read once. If we have `(Move &y &x _)`, and `y` is never readed, and `x` is readed only by this `Move`. When `elimDeadAutosGeneric` returns, both the `Move` and `y` were eliminated, then `x` becomes never readed. So the only thing that we need to do is to find `x` and make sure it is only readed by the `Move` that is going to be eliminated (ensured by `rely`), then treat it as never readed before performing the elimination (by delete it from `used`). Am I missing something?
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Sep 30, 2025, 5:40:51 AMSep 30
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 1, 2025, 8:38:20 AMOct 1
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 7, 2025, 6:30:45 AMOct 7
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Oct 8, 2025, 12:10:10 PMOct 8
to Youlin Feng, goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 11 (Latest):
Keith Randall . unresolved
I feel like this is overly complicated.
How about a possible simplification idea: We could instead treat moves as not uses. Then keep a map:
```
// There is a move that copies from moveSource[x] to x.
moveSource [*ir.Name]*ir.Name
```
Then, when we add `x` to `used`, we also add `moveSource[x]` to `used` (and recursively, if needed).
File src/cmd/compile/internal/ssa/deadstore.go
Line 294, Patchset 11 (Latest):
// and the name's class is also ir.PAUTO.Keith Randall . unresolved
This is not checked here. Does it matter?
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 8, 2025, 9:25:41 PMOct 8
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 2 comments![]( "Open in Gerrit")
Patchset-level comments
Keith Randall . unresolved
I feel like this is overly complicated.
How about a possible simplification idea: We could instead treat moves as not uses. Then keep a map:
```
// There is a move that copies from moveSource[x] to x.
moveSource [*ir.Name]*ir.Name
```Then, when we add `x` to `used`, we also add `moveSource[x]` to `used` (and recursively, if needed).
Youlin Feng
Good idea. I'll have a try.
File src/cmd/compile/internal/ssa/deadstore.go
Line 294, Patchset 11 (Latest):
// and the name's class is also ir.PAUTO.Keith Randall . resolved
This is not checked here. Does it matter?
Youlin Feng
I think I don't need to bother here, the value is already checked before adding it to `addr` or `elim`.
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 8, 2025, 9:31:07 PMOct 8
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #12 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 8, 2025, 9:33:40 PMOct 8
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
Youlin FengI feel like this is overly complicated.
How about a possible simplification idea: We could instead treat moves as not uses. Then keep a map:
```
// There is a move that copies from moveSource[x] to x.
moveSource [*ir.Name]*ir.Name
```Then, when we add `x` to `used`, we also add `moveSource[x]` to `used` (and recursively, if needed).
Good idea. I'll have a try.
Youlin Feng
Done.
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 8, 2025, 9:58:17 PMOct 8
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #13 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 8, 2025, 9:59:35 PMOct 8
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 14, 2025, 9:25:39 AMOct 14
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Youlin Feng . resolved
PTAL when it is convenient.
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Oct 14, 2025, 4:46:02 PMOct 14
to Youlin Feng, goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall added 4 comments![]( "Open in Gerrit")
Patchset-level comments
Keith Randall . resolved
Thanks, this version looks a lot simpler.
File src/cmd/compile/internal/ssa/deadstore.go
Line 284, Patchset 13 (Latest):
// If the addr of n is used by an OpMove as its source arg,
// and the OpMove's target arg is the addr of a name, thenKeith Randall . unresolved
I see no distinction between source and target here. How does that work?
Line 288, Patchset 13 (Latest):
move[nam] = append(move[nam], n)Keith Randall . unresolved
If we loop multiple times in the iteration loop below, will we end up adding the same entry multiple times?
Line 358, Patchset 13 (Latest):
// Propagate "used" by OpMoves.Keith Randall . unresolved
Maybe this separate loop isn't necessary? If we had
```
var usedAdd func(n *ir.Name)
usedAdd = func(n *ir.Name) {
used.Add(n)
for _, source := range move[n] {
usedAdd(source)
}
}
```
Then we can replace all calls of `used.Add(n)` with `usedAdd(n)`.
(And maybe we can put the `!used.Has(n)` and `changed = true` parts in here also.)
Then the iterate-until-converged loop we already have would do this for us.
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 14, 2025, 11:17:02 PMOct 14
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #14 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 14, 2025, 11:53:58 PMOct 14
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
3 comments
File src/cmd/compile/internal/ssa/deadstore.go
Line 284, Patchset 13:
// If the addr of n is used by an OpMove as its source arg,
// and the OpMove's target arg is the addr of a name, thenKeith Randall . resolved
I see no distinction between source and target here. How does that work?
Youlin Feng
There is a slice operation on `args` in the upper switch statement when v.Op is OpMove, which removed `arg[0]`.
If we loop multiple times in the iteration loop below, will we end up adding the same entry multiple times?
Youlin Feng
Yes. So, I changed the value type of the `move` map to *ir.NameSet.
Maybe this separate loop isn't necessary? If we had
```
var usedAdd func(n *ir.Name)
usedAdd = func(n *ir.Name) {
used.Add(n)
for _, source := range move[n] {
usedAdd(source)
}
}
```
Then we can replace all calls of `used.Add(n)` with `usedAdd(n)`.(And maybe we can put the `!used.Has(n)` and `changed = true` parts in here also.)
Then the iterate-until-converged loop we already have would do this for us.
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 15, 2025, 9:22:57 AMOct 15
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Keith Randall (Gerrit)
Oct 15, 2025, 12:33:55 PMOct 15
to Youlin Feng, goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall added 2 comments![]( "Open in Gerrit")
Patchset-level comments
Keith Randall . unresolved
Code looks good. I think we do need some correctness tests, though. That we keep the source of a move in various situations. For instance, when we use a move's result, but the source of that move is a phi, make sure that both the phi's sources are marked used as well.
File src/cmd/compile/internal/ssa/deadstore.go
Line 309, Patchset 15 (Latest):
if !s.Has(n) {Keith Randall . unresolved
You don't need this guard. `Add` will do nothing if `n` is already in `s`.
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 15, 2025, 9:00:03 PMOct 15
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #16 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 15, 2025, 9:36:58 PMOct 15
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #17 to this change.
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 15, 2025, 10:03:47 PMOct 15
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
Code looks good. I think we do need some correctness tests, though. That we keep the source of a move in various situations. For instance, when we use a move's result, but the source of that move is a phi, make sure that both the phi's sources are marked used as well.
Youlin Feng
Added some.
File src/cmd/compile/internal/ssa/deadstore.go
You don't need this guard. `Add` will do nothing if `n` is already in `s`.
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Oct 16, 2025, 4:33:57 PMOct 16
to Youlin Feng, goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Youlin FengCode looks good. I think we do need some correctness tests, though. That we keep the source of a move in various situations. For instance, when we use a move's result, but the source of that move is a phi, make sure that both the phi's sources are marked used as well.
Added some.
Keith Randall
It looks like you added some more optimization tests, that we do apply the optimization when it is safe.
What I want is tests that make sure we don't do the optimization when it isn't safe to do.
Those wouldn't be test/codegen tests, but probably something in cmd/compile/internal/test.
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 17, 2025, 10:39:52 AMOct 17
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #18 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 17, 2025, 10:41:29 AMOct 17
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- 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. |
Youlin Feng (Gerrit)
Oct 17, 2025, 10:54:31 AMOct 17
to goph...@pubsubhelper.golang.org, Go LUCI, David Chase, Keith Randall, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Youlin FengCode looks good. I think we do need some correctness tests, though. That we keep the source of a move in various situations. For instance, when we use a move's result, but the source of that move is a phi, make sure that both the phi's sources are marked used as well.
Keith RandallAdded some.
It looks like you added some more optimization tests, that we do apply the optimization when it is safe.
What I want is tests that make sure we don't do the optimization when it isn't safe to do.
Those wouldn't be test/codegen tests, but probably something in cmd/compile/internal/test.
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Oct 21, 2025, 6:20:10 PMOct 21
to Youlin Feng, goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall voted and added 3 comments![]( "Open in Gerrit")
Votes added by Keith Randall
| Code-Review | +2 |
3 comments
Patchset-level comments
File src/cmd/compile/internal/ssa/deadstore.go
Line 208, Patchset 18 (Latest):
move := make(map[*ir.Name]*ir.NameSet) // for a (Move &y &x _) and y is unused, move[y].Add(x)Keith Randall . unresolved
I think this should just be a `ir.NameSet`, not a `*ir.NameSet`.
Line 306, Patchset 18 (Latest):
s = new(ir.NameSet)Keith Randall . unresolved
Then this is just s = ir.NameSet{}
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 22, 2025, 2:38:54 AMOct 22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from David Chase, Keith Randall and Youlin Feng
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Youlin Feng uploaded patch set #19 to this change.
Following approvals got outdated and were removed:
- Code-Review: +2 by Keith Randall
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Attention is currently required from:
- David Chase
- Keith Randall
- Youlin Feng
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. |
Youlin Feng (Gerrit)
Oct 22, 2025, 2:40:06 AMOct 22
to goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
2 comments
File src/cmd/compile/internal/ssa/deadstore.go
Line 208, Patchset 18:
move := make(map[*ir.Name]*ir.NameSet) // for a (Move &y &x _) and y is unused, move[y].Add(x)Keith Randall . resolved
I think this should just be a `ir.NameSet`, not a `*ir.NameSet`.
Youlin Feng
Done
Then this is just s = ir.NameSet{}
Youlin Feng
Done
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 23, 2025, 9:22:28 AM (14 days ago) Oct 23
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Youlin Feng . resolved
PTAL.
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Youlin Feng (Gerrit)
Oct 27, 2025, 9:25:15 AM (10 days ago) Oct 27
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Youlin Feng . resolved
PTAL when it is convenient.
Youlin Feng (Gerrit)
Oct 30, 2025, 8:54:11 AM (7 days ago) Oct 30
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Keith Randall
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- David Chase
- Keith Randall
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. |
Keith Randall (Gerrit)
Nov 3, 2025, 4:38:01 PM (3 days ago) Nov 3
to Youlin Feng, goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall voted![]( "Open in Gerrit")
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Keith Randall (Gerrit)
Nov 3, 2025, 4:38:15 PM (3 days ago) Nov 3
to Youlin Feng, goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Keith Randall voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Michael Pratt (Gerrit)
Nov 4, 2025, 3:45:04 PM (2 days ago) Nov 4
to Youlin Feng, goph...@pubsubhelper.golang.org, Michael Pratt, Keith Randall, Keith Randall, Go LUCI, David Chase, Gopher Robot, golang-co...@googlegroups.com
Attention needed from David Chase and Youlin Feng
Michael Pratt voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- David Chase
- Youlin Feng
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. |
Gopher Robot (Gerrit)
Nov 4, 2025, 3:46:20 PM (2 days ago) Nov 4
to Youlin Feng, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Michael Pratt, Keith Randall, Keith Randall, Go LUCI, David Chase, golang-co...@googlegroups.com
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
Commit message:
cmd/compile/internal/ssa: more aggressive on dead auto elim
Propagate "unread" across OpMoves. If the addr of this auto is only used
by an OpMove as its source arg, and the OpMove's target arg is the addr
of another auto. If the 2nd auto can be eliminated, this one can also be
eliminated.
This CL eliminates unnecessary memory copies and makes the frame smaller
in the following code snippet:
func contains(m map[string][16]int, k string) bool {
_, ok := m[k]
return ok
}
These are the benchmark results followed by the benchmark code:
goos: linux
goarch: amd64
cpu: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
Map1Access2Ok-8 9.582n ± 2% 9.226n ± 0% -3.72% (p=0.000 n=20)
Map2Access2Ok-8 13.79n ± 1% 10.24n ± 1% -25.77% (p=0.000 n=20)
Map3Access2Ok-8 68.68n ± 1% 12.65n ± 1% -81.58% (p=0.000 n=20)
package main_test
import "testing"
var (
m1 = map[int]int{}
m2 = map[int][16]int{}
m3 = map[int][256]int{}
)
func init() {
for i := range 1000 {
m1[i] = i
m2[i] = [16]int{15:i}
m3[i] = [256]int{255:i}
}
}
func BenchmarkMap1Access2Ok(b *testing.B) {
for i := range b.N {
_, ok := m1[i%1000]
if !ok {
b.Errorf("%d not found", i)
}
}
}
func BenchmarkMap2Access2Ok(b *testing.B) {
for i := range b.N {
_, ok := m2[i%1000]
if !ok {
b.Errorf("%d not found", i)
}
}
}
func BenchmarkMap3Access2Ok(b *testing.B) {
for i := range b.N {
_, ok := m3[i%1000]
if !ok {
b.Errorf("%d not found", i)
}
}
}
Fixes #75398
Change-Id: If75e9caaa50d460efc31a94565b9ba28c8158771
Reviewed-on: https://go-review.googlesource.com/c/go/+/702875
Reviewed-by: Keith Randall <k...@golang.org>
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Keith Randall <k...@golang.org>
Reviewed-by: Keith Randall <k...@google.com>
Reviewed-by: Michael Pratt <mpr...@google.com>
Files:
- M src/cmd/compile/internal/ssa/deadstore.go
- A src/cmd/compile/internal/test/move_test.go
- M test/codegen/maps.go
Change size: M
Delta: 3 files changed, 126 insertions(+), 20 deletions(-)
Branch: refs/heads/master
Submit Requirements:
Code-Review: +1 by Michael Pratt, +1 by Keith Randall, +2 by Keith Randall
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages