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: move arch-specific rewrite rules and ops into arch-specific packages #20104

Open
josharian opened this issue Apr 24, 2017 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance ToolSpeed
Milestone

Comments

@josharian
Copy link
Contributor

josharian commented Apr 24, 2017

Right now, all the .rules files, all the generated code, and all the ops are in package ssa. This monolith has a few downsides: It seems a bit semantically incorrect, and it contributes to issues like #20084, and it makes rebuilding the compiler slower when working on the ssa package.

I'd like to move all the arch-specific stuff into cmd/compile/internal/ARCH. This is non-trivial and does have costs. Rough proposed plan:

  1. Export all functions used by rewrite rules, so that they can be called from outside package ssa. There's a lot of this, including API surface we might normally keep private.

  2. Everywhere we refer to arch-specific ops in package ssa, add some form of generalization.

  3. Set up registration hooks for the value and block rewriters, must as we do now for SSAMarkMoves.

  4. Move .rules and ops files to package ssa and teach rulegen about that. Within the generated files, do import . "cmd/compile/internal/ssa" to avoid having the teach rulegen to do ssa.NewValue for arch rules but just NewValue for generic rules. This will also reduce churn in the rules files themselves.

I think that's all that needs doing, but we might find more along the way; I started on this but it was big enough I wanted to discuss first.

Comments?

cc @mdempsky @bradfitz @randall77

@randall77
Copy link
Contributor

Export all functions used by rewrite rules, so that they can be called from outside package ssa. There's a lot of this, including API surface we might normally keep private.

Some of these functions are arch-specific (e.g. fitsARM64Offset), so for those this would be a win.

Everywhere we refer to arch-specific ops in package ssa, add some form of generalization.

It's really nice to have all the ops in a single table & namespace. For instance, schedule.go:schedule references all of the LoweredGetClosurePtr ops. How would you do that in this new world? (Maybe a bad example, as the closure ptr ops probably deserve a separate flag in the opInfo struct.)

I'm not convinced this will be worth it. It's a fair amount of churn and at least I'm not feeling the pain of the current setup right now.

@josharian
Copy link
Contributor Author

How would you do that in this new world?

A flag or perhaps a function.

I'm not convinced this will be worth it.

Fair enough. I'll back-burner it for now.

It's a fair amount of churn and at least I'm not feeling the pain of the current setup right now.

FWIW, my main motivations is compile times--I think it could help significantly, particular as the number of supported architectures grow.

@randall77
Copy link
Contributor

randall77 commented Apr 25, 2017

FWIW, my main motivations is compile times--I think it could help significantly, particular as the number of supported architectures grow.

You mean compile time of the compiler, right? Not general compile time.

@josharian
Copy link
Contributor Author

You mean compile time of the compiler, right? Not general compile time.

Right. Cycle time for us.

@andybons andybons added NeedsFix The path to resolution is known, but the work has not been done. Performance labels Apr 11, 2018
@andybons andybons added this to the Unplanned milestone Apr 11, 2018
@josharian
Copy link
Contributor Author

Inspired by #27739, I took another quick and dirty stab at this. I left the ops in package SSA and moved only generated code for rewrite rules. There's an unsolved problem around constructing ssa configs for ssa tests, but the rest works and passes toolstash-check -all. It speeds up make.bash on my laptop by 10%, despite the fact that it also disabled a bootstrapping optimization to avoid compiling rules for other architectures during initial optimization. Restoring that would further speed up make.bash.

@josharian
Copy link
Contributor Author

Given #27739 I think it may be worth actually doing this in 1.14.

cc @mvdan as FYI since he’s poking around in this code

@josharian josharian modified the milestones: Unplanned, Go1.14 May 9, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@mknyszek mknyszek moved this to Triage Backlog in Go Compiler / Runtime Jul 15, 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. NeedsFix The path to resolution is known, but the work has not been done. Performance ToolSpeed
Projects
Status: Triage Backlog
Development

No branches or pull requests

6 participants