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

cmd/compile: Increasing complexity when inlining #69925

Closed
DragonDev1906 opened this issue Oct 17, 2024 · 4 comments
Closed

cmd/compile: Increasing complexity when inlining #69925

DragonDev1906 opened this issue Oct 17, 2024 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@DragonDev1906
Copy link

Go version

go version go1.23.1 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/xxx/.cache/go-build'
GOENV='/home/xxx/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/xxx/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/xxx/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/xxx/go.mod'
GOWORK='/xxx/go.work'
CGO_CFLAGS='-I/snap/ego-dev/current/opt/ego/include'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-L/snap/ego-dev/current/opt/ego/lib -L/usr/lib/openssl-1.1'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2403704166=/tmp/go-build -gno-record-gcc-switches'

What did you do?

With the code below the following inlining decisions are made/shown:

godbolt

can inline (*BasicWallet).Address with cost 3 as: method(*BasicWallet) func() common.Address { return w.Addr }
can inline (*BasicWallet).Address2 with cost 6 as: method(*BasicWallet) func() common.Address { return (*BasicWallet).Address(w) }
can inline (*BasicWallet).Address3 with cost 9 as: method(*BasicWallet) func() common.Address { return (*BasicWallet).Address2(w) }

Although this works, it reduces the chance of something getting inlined, as it increases the complexity of the function something is inlined into, even though the resulting bytecode will/should be identical for all 3 methods (otherwise there would be no inlining).

What did you see happen?

(see above)

What did you expect to see?

Correct me if I'm wrong, but shouldn't these be something like the following, as that's actually what inlining does:

can inline (*BasicWallet).Address with cost 3 as: method(*BasicWallet) func() common.Address { return w.Addr }
can inline (*BasicWallet).Address2 with cost 3 as: method(*BasicWallet) func() common.Address { return w.Addr }
can inline (*BasicWallet).Address3 with cost 3 as: method(*BasicWallet) func() common.Address { return w.Addr }

Since inlining effectively reduces the effort/size of a function, these should probably not increase in complexity.

@DragonDev1906 DragonDev1906 changed the title Increasing complexity when inlining cmd/compile: Increasing complexity when inlining Oct 17, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 17, 2024
@mateusz834
Copy link
Member

Also same thing happens with field embeding, i once noticed that binary.NativeEndian has bigger inlining cost than binary.LittleEndian/binary.BigEndian, even though it looks like this:

type nativeEndian struct {
littleEndian
}
// NativeEndian is the native-endian implementation of [ByteOrder] and [AppendByteOrder].
var NativeEndian nativeEndian

@randall77
Copy link
Contributor

This is a consequence of deciding inlining complexity early in compilation, before we know how many instructions a function will actually take. The wrapper around the inline looks like it would require some cost, even though code generation can eliminate all of that cost.

Improvements in this area probably fall under the general umbrella of #61502.

@prattmic
Copy link
Member

@randall77 Is there anything specific we should keep this issue for, or shall we just close in favor of the more general #61502?

@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 21, 2024
@prattmic prattmic added this to the Backlog milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants