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: migrate amd64-specific optimizations to generic optimizations #16270

Closed
josharian opened this issue Jul 5, 2016 · 3 comments
Closed
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@josharian
Copy link
Contributor

josharian commented Jul 5, 2016

There are many optimizations in amd64.rules that apply to many architectures. As we add architectures, it'd be nice not to repeat discovering and encoding these rules.

To pick an example at random:

(ANDLconst [c] (ANDLconst [d] x)) -> (ANDLconst [c & d] x)

This could probably be done in generic.rules by introducing generic AndNNconst ops.

In some cases, we'd still need the arch-specific rules because optimizable patterns are introduced during lowering. If there are enough of those cases, it might be worth introducing common lowered ops and common lowered optimizations. The best way to find out it to start migrating rules and see.

cc @cherrymui @dr2chase @randall77 @brtzsnr

@josharian josharian added this to the Go1.8 milestone Jul 5, 2016
@randall77
Copy link
Contributor

randall77 commented Jul 5, 2016

We can do this in generic rules like:

(And32 (And32 x (Const32 [c])) (Const32 [d])) -> (And32 x (Const32 [c&d]))

(and the symmetries thereof.)

I think we should do these as much as possible in generic.rules. These rules were originally put in AMD64.rules because ANDL gets generated by the shift lowerings. That may not be the case for all architectures.

@brtzsnr
Copy link
Contributor

brtzsnr commented Jul 5, 2016

Regarding optimizing chains of AND/ADD/XOR/OR: there was a paper (I need to look it up again) that described how to do this for general case instead of having many rewrite rules for every usecase. We might want to implement that as separate pass.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 21, 2016
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017
@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
@cherrymui cherrymui modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 18, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 1, 2018
@randall77
Copy link
Contributor

I'm going to close this.
We do lots of constant folding in generic.rules now. For example, this rule handles the original report:

(And32  (Const32 [c])  (Const32 [d]))  -> (Const32 [int64(int32(c&d))])

I'm not sure what else might be done for this issue. Someone could do a pass to see if there are rules in the arch-specific files which are redundant with the generic rules, and where those lowered ops aren't introduced by other lowering rules. Seems not worth people's time.

It would be nice to have a general "rule can't ever fire" detector, which would catch this case and others. I had a rule coverage CL working at one point. It helped, but there's enough weird rules that all.bash doesn't cover everything (maybe it should, but that's a different issue).

@golang golang locked and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants