-
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: regression in TestIntendedInlining on ppc64x, others #22239
Comments
The ppc64 failures should be fixed by 53bbddd. The mips64 failures still need to be dealt with though. I think by updating TestIntendedInlining to expect nextFreeFast to not be inlinable on mips64. |
Likely that we forgot about this nextFreeFast edge case. Is this arch one of those builders that isn't a trybot? |
@mvdan Correct, MIPS64 is not a trybot architecture. The problem is nextFreeFast really is too expensive to inline on MIPS64, because we don't have an optimized version of Ctz64 on that arch. However, because of #19261, we weren't correctly accounting for how expensive Ctz64 is, so we were inlining nextFreeFast anyway. |
Change https://golang.org/cl/72271 mentions this issue: |
Since inlining budget calculation is fixed in CL 70151 runtime.nextFreeFast is no longer inlineable on MIPS64x because it does not support Ctz64 as intrinsic. Skip the test. Updates #22239. Change-Id: Id00d55628ddb4b48d27aebfa10377a896765d569 Reviewed-on: https://go-review.googlesource.com/72271 Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This problem is fixed on ppc64x. Not sure how to test mips64x or if that is what you are asking me to investigate. If someone can verify that the mips64x works, I can close it. |
Change https://golang.org/cl/227808 mentions this issue: |
Mark nextFreeFast as not inline, as it is too expensive to inline on riscv64. Also remove riscv64 from non-atomic inline architectures, as we now have atomic intrisics. Updates #22239 Change-Id: I6e0e72c1192070e39f065bee486f48df4cc74b35 Reviewed-on: https://go-review.googlesource.com/c/go/+/227808 Reviewed-by: Cherry Zhang <cherryyz@google.com> Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Is there anything remaining to be done for this issue? |
Timed out in state WaitingForInfo. Closing. (I am just a bot, though. Please speak up if this is a mistake or you have the requested information.) |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?go tip
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?ppc64le & ppc64
What did you do?
Noticed consistent failures on the golang.org/build page for ppc64le, ppc64
What did you expect to see?
No failures
What did you see instead?
ok cmd/compile 4.918s
--- FAIL: TestIntendedInlining (5.03s)
inl_test.go:210: runtime.nextFreeFast was not inlined: function too complex: cost 113 exceeds budget 80
FAIL
FAIL cmd/compile/internal/gc 17.646s
2017/10/11 23:22:15 Failed: exit status 1
This started with commit a509cae.
The text was updated successfully, but these errors were encountered: