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 uses of ir.Node with ir.Expr or ir.Stmt where appropriate #52731

Open
mdempsky opened this issue May 5, 2022 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented May 5, 2022

In go/ast, we use ast.Expr and ast.Stmt to represent when an ast.Node is known to be an expression or statement, respectively.

Within cmd/compile, we have analogous ir.Expr and ir.Stmt interface types, but for historical reasons most code still uses ir.Node. This issue lays out a plan for how to migrate the compiler to use them. (Having started a few attempts at this, it's clearly not something that can gracefully land in a single CL.)

  1. Introduce new ir.ExprNode and ir.StmtNode types. Normally, these will be type aliases to ir.Node; but if the strict-ir build tag is set, then they'll be aliases to ir.Expr and ir.Stmt, respectively, instead.
  2. Add a unit test (e.g., cmd/compile/internal/ir.TestStrictIR) that simply uses go/types to check that a (initially empty) list of packages all still type check successfully with the strict-ir build tag set.
  3. One package at a time, replace as many uses of ir.Node with ir.ExprNode or ir.StmtNode as possible (and refactoring code as needed), and then add the package to the unit test from step 2 to prevent backsliding.
  4. Once all cmd/compile packages have been transitioned, we change ir.ExprNode and ir.StmtNode to be aliases to ir.Expr and ir.Stmt unconditionally (i.e., make strict-ir the default/only mode), and remove TestStrictIR too.
  5. Global rewrite CL removing ir.ExprNode and ir.StmtNode and replacing all uses with ir.Expr and ir.Stmt. (This could be split into 2 or more steps if we're concerned about conflicts with in-flight CLs; but I think as long as we give heads up before landing the CL, it should be fine.)

I don't think this is pressing work, but wanted to lay out a plan and start getting buy-in from the rest of the compiler team before embarking upon it.

Two possible cons to this plan to note:

  1. Interface-to-interface type conversions (e.g., Expr->Node or Stmt->Node) require a runtime conversion to compute the itab, so it's possible this ends up slowing down some code paths. This is something worth measuring between steps 3 and 4. (Any ideas on how to measure the possible impact here before doing all the work would be welcome.)
  2. Because we can't use generics in the compiler, we'll probably have to duplicate some []ir.Node code paths to also operate on []ir.Expr and []ir.Stmt.

It's worth noting though that both of these cons apply to go/ast already, and I've not heard them raised before. But then Go 1 compatibility also precludes any work that could address these issues.

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 5, 2022
@mdempsky mdempsky added this to the Backlog milestone May 5, 2022
@mdempsky mdempsky changed the title cmd/compile: replace ir.Node with ir.Expr or ir.Stmt where appropriate cmd/compile: replace uses of ir.Node with ir.Expr or ir.Stmt where appropriate May 5, 2022
@griesemer
Copy link
Contributor

Presumably the goal here is to increase static type safety and with that increased readability, and possibly a reduction of the node sizes. Do I get this right?

If so, I think it's a reasonable plan.
Node size reduction is always good as we know from past experience that smaller nodes result in a faster compiler.

I'm not sure why you refer to go/ast since the compiler is using the syntax package which does have various (if small) improvements over go/ast. Presumably the new ir nodes would mimic the syntax package's Expr and Stmt node? (I think the useful differences are mostly on the Expr side.)

@mdempsky
Copy link
Member Author

mdempsky commented May 6, 2022

Presumably the goal here is to increase static type safety and with that increased readability, and possibly a reduction of the node sizes. Do I get this right?

Correct. Though I don't anticipate any node size changes, since Expr/Stmt are still interfaces?

If so, I think it's a reasonable plan.

Thanks!

I'm not sure why you refer to go/ast since the compiler is using the syntax package which does have various (if small) improvements over go/ast.

Only for general reference. Neither go/ast nor syntax are directly involved in this plan, and I think even within the compiler team more people are familiar with go/ast than syntax.

Presumably the new ir nodes would mimic the syntax package's Expr and Stmt node?

Just to be clear, package ir already has interface types Node/Expr/Stmt and a variety of concrete *Expr and *Stmt struct types, like go/ast and syntax: https://pkg.go.dev/cmd/compile/internal/ir. (This was the refactoring that Russ led in Dec 2020.)

It's just that the automated refactoring left us still using Node everywhere, but in many places Expr or Stmt would be clearer/safer.

@mdempsky
Copy link
Member Author

mdempsky commented May 6, 2022

@aclements reminds that most of our builders are still bootstrapping with Go 1.4, which doesn't support type aliases. So the plan as stated would need to wait for #44505 to be fixed first.

cmd/dist already rewrites source files for building toolchain1. It could be taught to rewrite ExprNode/StmtNode into Node too, so we don't depend on type aliases during bootstrapping.

I'll explore for less hacky solutions.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: Triage Backlog
Development

No branches or pull requests

3 participants