-
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: make rulegen produce less verbose code #33644
Comments
I also wonder if some lines, such as |
I would like to see a way to indicate that a rule should not be expanded to generate all possible operand orderings even though the opcode is marked commutative. The main example where this is a problem is in the rules to combine loads and stores which currently exist for many GOARCH targets. Those rules consist of multiple ORs, the statement to be matched can really only occur with a few of the possible operand orders, and yet all combinations are generated for all possible orders. This is especially costly for the 8 byte cases. This would reduce the size of the rewrite files, the compile binary, and compile time. |
That's an interesting idea, thanks. Could this be done automatically, or would the rules syntax need extra information to know when or not to do this expansion? |
I'm not sure this could be done automatically, so I was thinking of some syntax to add to the rule, like maybe a modifier to the opcode or a prefix with a special name that could be added to the outermost level to restrict the number of combinations generated. In the case I've mentioned with combined loads and stores, we would probably want to allow just 2 combinations to be generated: the operands in the order they appear in the rule, and the reverse order. That way it could still match either of these for the 4 byte case:
It is highly unlikely that this statement would be written with the operands in a different order from these, yet many combinations are generated and matches are attempted. I'd be willing to try out something to see how it helps. We currently do not have any of the combined load and store rules for ppc64 big endian, and we are missing some for the 4 byte cases on ppc64le, but I've avoided adding them because of how much they will increase the size of rewritePPC64.go. |
I agree with Lynn that reducing the amount of code generated for commutative operations would be really great and a big code size win. I tried writing generic rules for unaligned loads and the 64-bit load generated megabytes of code due to the 7 commutative ops. My attempt to use generic rules to match variable rotates also ran into this combinatorial explosion. As well as the approach suggested we could also look at swapping arguments to commutative operations so that they are put into a canonical order (I think there was a CL where Ian mentioned this is how GCC deals with this issue). Another approach we could investigate is swapping argument values in the generated code. For example, I think in a lot of cases we could generate code that looked like this (assuming v is commutative): ...
v1 := v.Args[0]
v2 := v.Args[1]
if v1.Op != OpBlah {
v1, v2 = v2, v1
}
if v1.Op != OpBlah {
break
}
... This only works when we know that the arguments to a value have different opcodes though (i.e. if in the above example both |
I did try a change to add something to a rule to indicate that it should not commute its operands. It reduced the size of the rewritePPC64.go file by about 1/3. However I ran into 2 cases that no longer match because the order of operands did not match the order of operands from the source statement, so some kind of reordering would be needed. I do recall that @ianlancetaylor made a comment about putting arguments in canonical order for this situation but I'm not sure on the details of how to implement it. (Force them in the order that they appear in the original source code? Or some other ordering.) |
Not the order of the original source code, no. The issue arises when trying to match sequences like (plus (plus a b) c). The idea is to pick some canonical order that you use when writing the rules, like, the deeper tree is always first. Then when comparing against the rules, you check the values, and if the deeper tree is second, you swap them before comparing. Of course this can only be used for commutative operators. |
OK, got it. That's what Mike was probably trying to explain above but I didn't get it. |
Change https://golang.org/cl/196498 mentions this issue: |
First, renove unnecessary "// cond:" lines from the generated files. This shaves off about ~7k lines. Second, join "if cond { break }" statements via "||", which allows us to deduplicate a large number of them. This shaves off another ~25k lines. This change is not for readability or simplicity; but rather, to avoid unnecessary verbosity that makes the generated files larger. All in all, git reports that the generated files overall weigh ~200KiB less, or about 2.7% less. While at it, add a -trace flag to rulegen. Updates #33644. Change-Id: I3fac0290a6066070cc62400bf970a4ae0929470a Reviewed-on: https://go-review.googlesource.com/c/go/+/196498 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Change https://golang.org/cl/196803 mentions this issue: |
First, be consistent about declaring typ as &b.Func.Config.Types and not &config.Types. Not particularly better, and it barely changes the output, but we're more consistent now. Second, remove a bit of duplication when handling the typ, auxint, and aux variables. Third and last, remove a stray canFail assignment; we ended up setting that in add, not breakf, so it's not necessary to set it manually if we don't use breakf. Updates #33644. Change-Id: I75999cb223a201969266fbfeae043599fa27fac5 Reviewed-on: https://go-review.googlesource.com/c/go/+/196803 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
I think we can open a new issue if we find a way to make the idea work. Unless @mundaym wants to keep this one open. |
Happy to close this. |
This is a continuation to #30810, once the rewrite has been merged. A number of ideas will be proposed below. The purpose of this is to:
Here are my ideas so far:
Simplifying of boolean expressions. For example, turn
!(d == 32-c)
intod != 32-c
.Joining of contiguous break conditions, such as:
cond
line here:These are the ideas I have so far. If we think any of them would reduce readability, they can be dropped. I'm also open to more ideas and suggestions, of course. I'd send these as separate CLs so we can measure their impact on the generated code separately.
/cc @josharian @randall77 @laboger @mundaym @cherrymui
The text was updated successfully, but these errors were encountered: