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

Open
laboger opened this Issue Oct 12, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@laboger
Contributor

laboger commented Oct 12, 2017

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.

@randall77

This comment has been minimized.

Contributor

randall77 commented Oct 12, 2017

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 12, 2017

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.

@mvdan

This comment has been minimized.

Member

mvdan commented Oct 13, 2017

Likely that we forgot about this nextFreeFast edge case. Is this arch one of those builders that isn't a trybot?

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 13, 2017

@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.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 20, 2017

Change https://golang.org/cl/72271 mentions this issue: cmd/compile: skip runtime.nextFreeFast inlining test on MIPS64x

gopherbot pushed a commit that referenced this issue Oct 20, 2017

cmd/compile: skip runtime.nextFreeFast inlining test on MIPS64x
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>

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@laboger

This comment has been minimized.

Contributor

laboger commented Apr 5, 2018

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.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment