-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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: performance regression in 1.20 #57505
Comments
cc @golang/runtime |
I can reproduce the regression on It looks to me that with Go 1.19 or non-unified, it inlines cc @mdempsky |
Yeah, if I remove the type parameters (hard code |
It is weird that Vec_Dot/Vec4 is not reproducible. Nevertheless, in https://github.com/polyred/polyred/tree/develop/math, there are more regression examples:
|
This is also multi-level inlining. E.g. standard |
I'm on vacation (and currently on a plane), but briefly looking at the compiler's -m and -S output, it looks like everything is inlining the same. I don't see anything obviously wrong. (Caveat: I had to retype stuff from my phone onto my laptop and I simplified things slightly because of that.) I'll take a look once I'm back in the office on Monday. |
Hmmm, I got different results with tip vs. 1.19 or non-unified. With Go 1.19,
With tip,
but no In particular,
|
@cherrymui Thanks, I'm able to repro the issue now. Not sure what went wrong with my earlier attempt. |
The issue here is that unified IR has a simpler heuristic for deciding which function bodies to re-export. It simply re-exports functions that were inlined into the current compilation unit. (It also always exports its own inlinable functions.) The problem manifests here that mymodule/math doesn't actually instantiate its generic types/functions, so math.{Abs,Float64bits,Float64frombits} never get inlined within that package, so they're never re-exported by that package either. Then when compiling mymodule/math_test, the inline bodies aren't available so they don't get inlined. Two possible workarounds:
There's supposed to be a compiler diagnostic to warn when this happens. I'm not sure at the moment why it's not firing. |
Hey @mdempsky, doing a sweep of the Go 1.21 milestone. Any updates here? Should this go into Backlog? Thanks. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes, also in 1.20rc1
What did you do?
What did you expect to see?
Same performance.
What did you see instead?
The text was updated successfully, but these errors were encountered: