Skip to content
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

Jump to bottom

cmd/compile: regression in TestIntendedInlining on ppc64x, others #22239

Closed
laboger opened this issue Oct 12, 2017 · 9 comments
Closed

cmd/compile: regression in TestIntendedInlining on ppc64x, others #22239

laboger opened this issue Oct 12, 2017 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Backlog

Comments

@laboger
Copy link
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
Copy link
Contributor

@mdempsky

@mdempsky
Copy link
Member

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
Copy link
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
Copy link
Member

@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
Copy link

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
@cherrymui
cmd/compile: skip runtime.nextFreeFast inlining test on MIPS64x
5c18a3c 
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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@laboger
Copy link
Contributor Author

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
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

Change https://golang.org/cl/227808 mentions this issue: cmd/compile: update TestIntendedInlining for riscv64

gopherbot pushed a commit that referenced this issue Apr 13, 2020
@4a6f656c
cmd/compile: update TestIntendedInlining for riscv64
d3d21d0 
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>
@bcmills
Copy link
Contributor

bcmills commented Mar 31, 2022

Is there anything remaining to be done for this issue?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 31, 2022
@gopherbot
Copy link

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

@rsc rsc unassigned laboger Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
9 participants
@mdempsky @rsc @andybons @ianlancetaylor @mvdan @bcmills @randall77 @gopherbot @laboger