-
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: same global variable loaded twice for function call argument #28484
Comments
This seems like a great issue to get one's hands dirty with the compiler during the freeze. @josharian @martisch - Could you throw me a bone on where to start looking to investigate something like this ?
Apologies for a couple of beginner questions - Should this be in some SSA pass ? (if so, which one ?) Should this be in a rewrite rule ? Should this be a new pass ? |
Hard to say without digging into it. I'd hope it could be a generic rewrite rule. I'd start by poking around ssa.html and understanding in detail the example code and seeing if you see a clear, generalizable way to fix it. However, as I was recently reminded by @mundaym, rewriting rule (and any SSA work) around memory operations are really tricky to get right. So this might prove not to be an ideal first place to experiment. |
Thanks ! Yes I have been looking into the ssa.html. Essentially, we have something like this -
What we would want is something like this -
I am not sure if this can be a rewrite rule. IIUC, we would need to check all params of a As you say, will leave it for somebody else. |
At a glance, it seems to me that one route is to use alias analysis (currently called func disjoint) to recognize that v7 and v9 don’t overlap, then float the load before the store, and then eliminate the duplicate loads. Or some such memory re-ordering. Related: It might also be worth looking at Philip Hofer’s old CL that brings alias analysis to the tighten pass. I wanted to get that revived this cycle but didn’t get to it. |
Since it doesn't look like this is planned for Go 1.14, I wouldn't mind digging into the SSA to see what I can find. I plan to hack on this, unless someone feels this issue is more pressing and deserves someone with more experience. |
@josharian can you point me to "Philip Hofer’s old CL"? Perhaps I can take a look at it. |
I think there is an extra constraint on this. Suppose we have a function var L = 0
var B bool
func sideEffect() int // {
// L = L+1
// return L
//}
//go:noinline
func takesTwo(i, j, k int) bool {
return i < j+k
}
func main() {
B = takesTwo(L, sideEffect(), L)
} According to go spec:
So originally, I thought 1st
Therefore, we can hoist
|
@danscales is looking into redoing how we marashal/unmarshal arguments and return values. He was thinking about pushing the actual generation of the stores/loads later into SSA. It would solve this issue as a side effect because the stores of arguments would not be there during CSE, so the two loads would be CSEd. |
@danscales I’m thrilled you are looking at this. If you are just getting started, the commit message from 2578ac5 might be helpful. |
With the late-call expansion by @dr2chase, this now does only one load:
|
What version of Go are you using (
go version
)?go version devel +2e9f0817f0 Tue Oct 30 04:39:53 2018 +0000 darwin/amd64
found this when benchmarking runtime.makeslice with make([]T, LenVar, LenVar) where LenVar was a global variable. However the issue can be shown with any function call where two arguments are loaded from the same global variable. The issue of unnecessary loading variables to often might come up in other contexts too. Filling for further investigation and possible general optimization.
Example:
main.L is loaded twice to write the arguments for takesTwo to the stack:
expected:
The text was updated successfully, but these errors were encountered: