Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: replace ir.Node with *ir.Name or *ir.Func where appropriate #42982

Closed
mdempsky opened this issue Dec 3, 2020 · 15 comments
Closed
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Dec 3, 2020

On the dev.regabi branch, there's now a lot of code that uses ir.Node when really *ir.Name or *ir.Func would be sufficient, clearer, and more efficient. We should change those where possible.

For example, both of these fields only ever store *ir.Name:

vars []ir.Node
idx map[ir.Node]int32

As basic steps, I'd recommend:

  1. Pick a slice or map that appears to only be used for variables (or functions) and change it from ir.Node to *ir.Name (or *ir.Func).

  2. Add type assertions and type conversions as necessary to get it to compile.

  3. Check that it still works correctly.

  4. Rewrite function signatures to push the type assertions as far out as possible.

Beware: Sometimes code may mostly store Names, but still occasionally store other Nodes. For example, state.vars says it's only used for ONAMEs, but then OANDAND/OOROR nodes end up getting stuffed in there too.

I'd recommend for contributors to start small. Maybe a few variables. Ideally sticking to just one file at a time. Also, coordinate here to avoid redundant work, or to ask questions if you're unsure.

/cc @cuonglm @zephyrtronium @rsc

@mdempsky mdempsky added Suggested Issues that may be good for new contributors looking for work to do. help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Dec 3, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275352 mentions this issue: [dev.regabi] cmd/compile: replace ir.Node with *ir.Name in Liveness

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275353 mentions this issue: [dev.regabi] cmd/compile: replace ir.Node with *ir.Name in Order

@mdempsky
Copy link
Contributor Author

mdempsky commented Dec 4, 2020

Actually, I think this can be largely automated using x/tools/go/pointer. So I recommend folks hold off until I've had a chance to try that.

gopherbot pushed a commit that referenced this issue Dec 4, 2020
Passes buildall w/ toolstash -cmp.

Updates #42982

Change-Id: Iad8df321adfd576da070c13ed16a9651d4e59ad8
Reviewed-on: https://go-review.googlesource.com/c/go/+/275352
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Dec 4, 2020
Passes buildall w/ toolstash -cmp.

Updates #42982

Change-Id: I7121c37f72ccbc141a7dd17fba1753f2c6289908
Reviewed-on: https://go-review.googlesource.com/c/go/+/275353
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@mdempsky
Copy link
Contributor Author

mdempsky commented Dec 4, 2020

x/tools/go/pointer seems like it could work, but it computes a lot more information than we need for this. So I'm going to try writing a custom SSA analyzer instead that should scale better for this use case.

I've also written github.com/mdempsky/gclint to identify cases that merit attention. You can run:

go install github.com/mdempsky/gclint@latest
gclint cmd/compile/internal/gc

to get the warnings.

There's also an -assign flag that warns about assignments of *ir.Name to ir.Node, but currently this is very noisy.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275754 mentions this issue: [dev.regabi] cmd/compile: change nowritebarrierrec to use map[*ir.Func]

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275753 mentions this issue: [dev.regabi] cmd/compile: replace NodeQueue with NameQueue

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275756 mentions this issue: [dev.regabi] cmd/compile: add ssa.Aux tag interface for Value.Aux

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275752 mentions this issue: [dev.regabi] cmd/compile: change NodeSet to NameSet

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275755 mentions this issue: [dev.regabi] cmd/compile: change iexport to avoid map[ir.Node]

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275757 mentions this issue: [dev.regabi] cmd/compile: change LocalSlot.N to *ir.Name

gopherbot pushed a commit that referenced this issue Dec 6, 2020
The only user of NodeSet (computing initialization dependencies) only
needs to store *Names in this structure. So change its definition to
match that need, and update the code in initorder.go accordingly.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I181a8aaf9bc71e88f4ac009c4f381a718080e48f
Reviewed-on: https://go-review.googlesource.com/c/go/+/275752
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Dec 6, 2020
Similar to the previous CL, the only two users of NodeQueue only
needed it for tracking objects, not arbitrary AST nodes. So change
it's signature to use *Name instead of Node.

This does require a tweak to the nowritebarrierrec checker, because
previously it was pushing the ODCLFUNC *Func pointers into the queue,
whereas now we push the ONAME/PFUNC *Name pointers instead. However,
it's trivial and safe to flip between them.

Also, this changes a handful of export-related code from Node to
*Name, to avoid introducing type assertions within iexport.go.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I867f9752121509fc3da753978c6a41d5015bc0ce
Reviewed-on: https://go-review.googlesource.com/c/go/+/275753
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Dec 6, 2020
All of the uses were already using *ir.Func index operands, so only
needs the map type itself updated.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I568d8601f3eb077e07e887f2071aa1a2667d803c
Reviewed-on: https://go-review.googlesource.com/c/go/+/275754
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Dec 6, 2020
In the past, we had a lot of trouble with misusing *types.Sym
throughout the frontend, so I tried to push us towards always passing
around ONAMEs instead. But for constructing and writing out the symbol
indexes for the indexed export data, keying by *types.Sym is exactly
what we want.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: Idd8f1fb057d75a52a34ebc7788d9332fb49caf8d
Reviewed-on: https://go-review.googlesource.com/c/go/+/275755
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275791 mentions this issue: [dev.regabi] cmd/compile: rewrite replace many ir.Nodes with *ir.Names

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275789 mentions this issue: [dev.regabi] cmd/compile: rewrite Aux uses of ir.Node to *ir.Name

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275790 mentions this issue: [dev.regabi] cmd/compile: ir.Node is no longer an ssa.Aux

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275788 mentions this issue: [dev.regabi] cmd/compile: introduce FwdRefAux for wrapping ir.Node as ssa.Aux

gopherbot pushed a commit that referenced this issue Dec 8, 2020
It's currently hard to automate refactorings around the Value.Aux
field, because we don't have any static typing information for it.
Adding a tag interface will make subsequent CLs easier and safer.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I41ae8e411a66bda3195a0957b60c2fe8a8002893
Reviewed-on: https://go-review.googlesource.com/c/go/+/275756
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Dec 8, 2020
This was already documented as always being an ONAME, so it just
needed a few type assertion changes.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I61f4b6ebd57c43b41977f4b37b81fe94fb11a723
Reviewed-on: https://go-review.googlesource.com/c/go/+/275757
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Dec 8, 2020
… ssa.Aux

OpFwdRef is the only SSA value that needs the ability to store an
arbitrary ir.Node in its Aux field. Every other SSA value always uses
an *ir.Name.

This CL introduces FwdRefAux, which wraps an ir.Node and implements
the ssa.Aux tag interface, so that a subsequent refactoring can change
ir.Node to not implement ssa.Aux.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: Id1475b28847579573cd376e82f28761d84cd1c23
Reviewed-on: https://go-review.googlesource.com/c/go/+/275788
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Dec 8, 2020
…nerated]

Now that the only remaining ir.Node implementation that is stored
(directly) into ssa.Aux, we can rewrite all of the conversions between
ir.Node and ssa.Aux to use *ir.Name instead.

rf doesn't have a way to rewrite the type switch case clauses, so we
just use sed instead. There's only a handful, and they're the only
times that "case ir.Node" appears anyway.

The next CL will move the tag method declarations so that ir.Node no
longer implements ssa.Aux.

Passes buildall w/ toolstash -cmp.

Updates #42982.

[git-generate]
cd src/cmd/compile/internal
sed -i -e 's/case ir.Node/case *ir.Name/' gc/plive.go */ssa.go

cd ssa
rf '
ex . ../gc {
  import "cmd/compile/internal/ir"

  var v *Value
  v.Aux.(ir.Node) -> v.Aux.(*ir.Name)

  var n ir.Node
  var asAux func(Aux)
  strict n        # only match ir.Node-typed expressions; not *ir.Name
  implicit asAux  # match implicit assignments to ssa.Aux
  asAux(n)        -> n.(*ir.Name)
}
'

Change-Id: I3206ef5f12a7cfa37c5fecc67a1ca02ea4d52b32
Reviewed-on: https://go-review.googlesource.com/c/go/+/275789
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Dec 8, 2020
After the previous rewrite, we can now remove CanBeAnSSASym and
CanBeAnSSAAux from the generic Node interface, and declare them just
on *ir.Name.

Updates #42982.

Change-Id: I865771fd30c95c009740410844f20ade08648343
Reviewed-on: https://go-review.googlesource.com/c/go/+/275790
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Dec 8, 2020
This commit adds exactly two "n := n.(*ir.Name)" statements, that are
each immediately preceded by a "case ir.ONAME:" clause in an n.Op()
switch. The rest of the changes are simply replacing "ir.Node" to
"*ir.Name" and removing now unnecessary "n.(*ir.Name)" type
assertions, exposing the latent typing details.

Passes buildall w/ toolstash -cmp.

Updates #42982.

Change-Id: I8ea3bbb7ddf0c7192245cafa49a19c0e7a556a39
Reviewed-on: https://go-review.googlesource.com/c/go/+/275791
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
@mdempsky
Copy link
Contributor Author

We're continuing to work on this, but I think this is largely done now. Remaining pieces are being picked up as they're found.

@golang golang locked and limited conversation to collaborators Jan 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

2 participants