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: regression: unnecessary spilling to the stack #61356
Comments
cc @golang/compiler @randall77 |
@dominikh noted in chat that the slice equivalent of this function still compiles at tip to pretty much the same thing as the 1.20 output above: https://go.dev/play/p/Ty9G09raoMM.go |
This is almost certainly the new scheduler: https://go-review.googlesource.com/c/go/+/270940 Both the new and the old scheduler basically have no heuristics here. There are lots of scheduleable values at one time and the scheduler choses kind of arbitrarily. In particular, the arbitrary choice between issuing another load and issuing a shift/or. I'm not sure why the slice version is different. Maybe the value numbering is different which causes a different arbitrary choice to be made. This problem is a bit tricky because I think we need to encode the size of the register set somehow. We do want to issue the loads early when there are only a few of them. But when there are lots of them we want to prioritize issuing uses of those loads. |
Ah, the difference between array and slice is from here: https://github.com/golang/go/blob/master/src/cmd/compile/internal/ssa/schedule.go#L68 |
Hm, looks like that special case isn't needed (anymore?). Also looks like I'm the one that added it. |
Change https://go.dev/cl/509856 mentions this issue: |
Another example that got worse with Go 1.21, even with the CL applied: https://go.dev/play/p/21VpDblOelZ.go (only the extra spilling is a regression, the other issues with the generated code already exist in Go 1.20.) |
That special case was needed to pass |
@dominikh That looks like a more general problem of the SETCS opcode (and friends) not having an indexed store version. |
Change https://go.dev/cl/510435 mentions this issue: |
Update #61356 Change-Id: I391af98563b1c068208784c80ea736c78c29639d Reviewed-on: https://go-review.googlesource.com/c/go/+/510435 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Martin Möhrmann <martin@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Martin Möhrmann <moehrmann@google.com>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
No.
What operating system and processor architecture are you using (
go env
)?Linux, amd64
What did you do?
Compile
What did you expect to see?
Output similar to that of Go 1.20.6:
What did you see instead?
Go 1.21 seems to be loading all of the values up front, only to spill some of them to the stack. This seems strictly worse than the old output.
/cc @prattmic
The text was updated successfully, but these errors were encountered: