-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
Change https://golang.org/cl/275352 mentions this issue: |
Change https://golang.org/cl/275353 mentions this issue: |
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. |
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>
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>
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:
to get the warnings. There's also an |
Change https://golang.org/cl/275754 mentions this issue: |
Change https://golang.org/cl/275753 mentions this issue: |
Change https://golang.org/cl/275756 mentions this issue: |
Change https://golang.org/cl/275752 mentions this issue: |
Change https://golang.org/cl/275755 mentions this issue: |
Change https://golang.org/cl/275757 mentions this issue: |
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>
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>
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>
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>
Change https://golang.org/cl/275791 mentions this issue: |
Change https://golang.org/cl/275789 mentions this issue: |
Change https://golang.org/cl/275790 mentions this issue: |
Change https://golang.org/cl/275788 mentions this issue: |
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>
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>
… 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>
…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>
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>
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>
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. |
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
:go/src/cmd/compile/internal/gc/plive.go
Lines 106 to 107 in 351bc2f
As basic steps, I'd recommend:
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).
Add type assertions and type conversions as necessary to get it to compile.
Check that it still works correctly.
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
The text was updated successfully, but these errors were encountered: