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: non-symmetric inline cost when using named vs non-named returns #38419

Open
qmuntal opened this issue Apr 13, 2020 · 3 comments
Open

cmd/compile: non-symmetric inline cost when using named vs non-named returns #38419

qmuntal opened this issue Apr 13, 2020 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Apr 13, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Dante\AppData\Local\go-build
set GOENV=C:\Users\Dante\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Dante\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\Dante\Documents\Code\go3mf\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Dante\AppData\Local\Temp\go-build018008438=/tmp/go-build -gno-record-gcc-switches

What did you do?

Code: https://play.golang.org/p/bM-akhk0QXG
go build -gcflags "-m=2"

What did you expect to see?

NewTriangleInline is equivalent to NewTriangleNoInline, the only difference is that the first uses a named return and the second one doesn´t, therefore I would expect that both have the same inline cost.

func NewTriangleInline(v1, v2, v3 uint32) (t Triangle) {
	t.SetIndex(0, v1)
	t.SetIndex(1, v2)
	t.SetIndex(2, v3)
	return
}
func NewTriangleNoInline(v1, v2, v3 uint32) Triangle {
	var t Triangle
	t.SetIndex(0, v1)
	t.SetIndex(1, v2)
	t.SetIndex(2, v3)
	return t
}

What did you see instead?

.\foo.go:11:6: can inline NewTriangleInline as: func(uint32, uint32, uint32) Triangle { t.SetIndex(0, v1); t.SetIndex(1, v2); t.SetIndex(2, v3); return  }
.\foo.go:18:6: cannot inline NewTriangleNoInline: function too complex: cost 84 exceeds budget 80
@andybons
Copy link
Member

andybons commented Apr 14, 2020 •
edited

@randall77 @josharian

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 14, 2020
@andybons andybons added this to the Unplanned milestone Apr 14, 2020
@randall77
Copy link
Contributor

randall77 commented Apr 14, 2020 •
edited

Yes, the inlining cost estimate is not very sophisticated. Explicit declarations count, implicit ones (args+returns) don't.
This particular problem can be fixed.

I worry about the more general issue though. The inlining cost model is just an estimate. It's always going to be wrong in some situations, even in otherwise silly ways (e.g. x := 5; y += x vs. y += 5).
To really understand the cost of the inlined function we'd need to compile for real, and that can be a lot more expensive (but we compile it anyway at some point, so maybe?). Above that, we really want to know the cost at the call site, which would depend on things like arguments which are constant.

So I guess the question is - where do we draw the line? What constitutes an inlining cost "bug" and what doesn't? Do we really want to fix this one bug, only to have lots of similar bugs dribble in over time?

I'm kind of inclined to say "not a bug". Even if we fix this bug, there's no guarantee we won't reweight the inliner next release and break your inlinings anyway. But I'm open to other ideas.

@randall77 randall77 added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 14, 2020
@qmuntal
Copy link
Contributor Author

qmuntal commented Apr 14, 2020 •
edited

@randall77 I agree with you, if the inline model is designed to give rough estimates, then this is not a bug but an improvement request.

I would also agree on closing this issue if a more generic one is created (if not there yet) for tracking a complete inliner. Else I´m inclined to leave it opened until this specific case is correctly managed.

Even if we fix this bug, there's no guarantee we won't reweight the inliner next release and break your inlinings anyway

Thanks for pointing me out this other issue. I will iterate some more time on NewTriangle to try to reduce its cost even more even if this request is implemented, as it is on my critical path. Anyway, this issue is not about fixing my specific code but to track the named returns issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@andybons @qmuntal @randall77