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: functions with type parameters cannot inline multiple levels deep across packages #56280

Closed
prattmic opened this issue Oct 17, 2022 · 6 comments
Closed

cmd/compile: functions with type parameters cannot inline multiple levels deep across packages #56280

prattmic opened this issue Oct 17, 2022 · 6 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge generics Issue is related to generics
Milestone
Go1.20

Comments

@prattmic
Copy link
Member

prattmic commented Oct 17, 2022 •
edited

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

$ go version
go version devel go1.20-da6042e82e Mon Oct 17 15:59:12 2022 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes (1.19)

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/usr/local/google/home/mpratt/.cache/go-build"
GOENV="/usr/local/google/home/mpratt/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/usr/local/google/home/mpratt/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/usr/local/google/home/mpratt/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/google-golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20-pre3 cl/474093167 +a813be86df"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/usr/local/google/home/mpratt/Downloads/inlinegenerics/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build512352002=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Build https://go.dev/play/p/X7fahXzzE38

What did you expect to see?

result = input.ViaGenericPageOffset inlined as a single AND.

What did you see instead?

input.ViaGenericPageOffset() is inlined as pkg.GenericPageOffset[example.com/pkg.Addr](input), even though the non-inline version of pkg.Addr.ViaGenericPageOffset fully inlines GenericPageOffset.

TEXT example.com/pkg.Addr.ViaGenericPageOffset(SB) /usr/local/google/home/mpratt/Downloads/inlinegenerics/pkg/pkg.go
  pkg.go:24             0x457600                25ff0f0000              ANDL $0xfff, AX         
  pkg.go:24             0x457605                c3                      RET                     

TEXT example.com/pkg.Addr.ViaConcretePageOffset(SB) /usr/local/google/home/mpratt/Downloads/inlinegenerics/pkg/pkg.go
  pkg.go:28             0x457620                25ff0f0000              ANDL $0xfff, AX         
  pkg.go:28             0x457625                c3                      RET                     

TEXT example.com/pkg.GenericPageOffset[go.shape.uintptr](SB) /usr/local/google/home/mpratt/Downloads/inlinegenerics/pkg/pkg.go
  pkg.go:14             0x457640                81e3ff0f0000            ANDL $0xfff, BX         
  pkg.go:14             0x457646                4889d8                  MOVQ BX, AX             
  pkg.go:14             0x457649                c3                      RET                     

TEXT main.main(SB) /usr/local/google/home/mpratt/Downloads/inlinegenerics/main.go
  main.go:12            0x457660                493b6610                CMPQ 0x10(R14), SP                                                              
  main.go:12            0x457664                767b                    JBE 0x4576e1                                                                    
  main.go:12            0x457666                4883ec18                SUBQ $0x18, SP                                                                  
  main.go:12            0x45766a                48896c2410              MOVQ BP, 0x10(SP)                                                               
  main.go:12            0x45766f                488d6c2410              LEAQ 0x10(SP), BP
# result = input.ViaConcretePageOffset()
  main.go:13            0x457674                488b0dc58a0900          MOVQ main.input(SB), CX                                                         
  pkg.go:28             0x45767b                90                      NOPL                                                                            
  pkg.go:18             0x45767c                81e1ff0f0000            ANDL $0xfff, CX                                                                 
  main.go:13            0x457682                48890dbf8a0900          MOVQ CX, main.result(SB)      
# result = input.ViaGenericPageOffset()
  main.go:14            0x457689                488b1db08a0900          MOVQ main.input(SB), BX                                                         
  pkg.go:24             0x457690                488d05593b0200          LEAQ example.com/pkg..dict.GenericPageOffset[example.com/pkg.Addr](SB), AX      
  pkg.go:24             0x457697                e8a4ffffff              CALL example.com/pkg.GenericPageOffset[go.shape.uintptr](SB)                    
  main.go:14            0x45769c                488905a58a0900          MOVQ AX, main.result(SB)          
...

If the contents of pkg are moved into package main (https://go.dev/play/p/8WY0mElNpV3), then input.ViaGenericPageOffset is inlined into main.main exactly the same as input.ViaConcretePageOffset. I do not expect this behavior to depend on which package the code is in.
@prattmic prattmic added the generics Issue is related to generics label Oct 17, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 17, 2022
@prattmic
Copy link
Member Author

cc @golang/compiler

@prattmic
Copy link
Member Author

prattmic commented Oct 17, 2022 •
edited

This feels like an export data issue to me. I see:

./main2.go:27:6: can inline GenericPageOffset[go.shape.uintptr] with cost 4 as: func(*[1]uintptr, go.shape.uintptr) go.shape.uintptr { return x & pageMask }
./main2.go:37:6: can inline Addr.ViaGenericPageOffset with cost 10 as: method(Addr) func() Addr { return GenericPageOffset[go.shape.uintptr](&.dict.GenericPageOffset[main.Addr], v) }

But only the second actually happens across packages. Is the inline body of GenericPageOffset[go.shape.uintptr] not included in export data?

@mdempsky mdempsky self-assigned this Oct 17, 2022
@mdempsky
Copy link
Member

Is the inline body of GenericPageOffset[go.shape.uintptr] not included in export data?

Instantiated bodies are not included in unified IR's export data format. It only exports generic bodies, and then reinstantiates them as appropriate. Similarly, it also reanalyzes for inlining and escape analysis.

The problem here is an imported, non-generic, inlinable function F calls a generic, inlinable function G[uintptr]. However, this isn't discovered until we get to inlining, lazily read in F's function body, and re-discover the call to G[uintptr] and instantiate it (as G[go.shape.uintptr], along with an appropriate runtime dictionary). At this point, we've already missed the opportunity to call CanInline on G[go.shape.uintptr] (though probably even if we created it earlier, ir.VisitFuncsBottomUp wouldn't realize it's transitively relevant to inlining F).

Probably we need to manually call CanInline on instantiated, imported generic functions.

@gopherbot
Copy link

Change https://go.dev/cl/443535 mentions this issue: cmd/compile: fix transitive inlining of generic functions

@mknyszek mknyszek added this to the Go1.20 milestone Oct 19, 2022
@HubertZhang
Copy link

Will this bugfix be backported to go 1.19?

@randall77
Copy link
Contributor

No, normally a performance fix does not qualify for a backport.
https://github.com/golang/go/wiki/MinorReleases

@cherrymui cherrymui mentioned this issue Dec 29, 2022
Open
@copybara-service copybara-service bot mentioned this issue Feb 3, 2023
Closed
copybara-service bot pushed a commit to google/gvisor that referenced this issue Feb 7, 2023
@nixprime @gvisor-bot
Make hostarch.Page/HugePage/CacheLine* functions generic and canonical.
0721ca2 
Also add the missing HugePage equivalents of Page functions.

The fix for golang/go#56280, which first appears in Go 1.20, is needed to
prevent this CL from regressing performance:

Before fix:
```
TEXT gvisor/pkg/sentry/mm/mm.(*MemoryManager).SetNumaPolicy(SB) gvisor/pkg/sentry/mm/syscalls.go
  ...
  addr.go:75            0x7f000054e35e          488d053bc2b100          LEAQ gvisor/pkg/hostarch/hostarch..dict.IsPageAligned[gvisor/pkg/hostarch/hostarch.Addr](SB), AX
  addr.go:75            0x7f000054e365          e8961cc4ff              CALL gvisor/pkg/hostarch/hostarch.IsPageAligned[go.shape.uintptr](SB)
```

After fix:
```
TEXT gvisor/pkg/sentry/mm/mm.(*MemoryManager).SetNumaPolicy(SB) gvisor/pkg/sentry/mm/syscalls.go
  ...
  addr.go:75            0x7f00004da61a          90                      NOPL
  addr.go:75            0x7f00004da61b          0f1f440000              NOPL 0(AX)(AX*1)
  sizes_util.go:57      0x7f00004da620          48f7c3ff0f0000          TESTQ $0xfff, BX
  syscalls.go:1021      0x7f00004da627          0f855f010000            JNE 0x7f00004da78c
```

PiperOrigin-RevId: 507608080
@golang golang locked and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge generics Issue is related to generics
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
6 participants
@mdempsky @mknyszek @prattmic @HubertZhang @randall77 @gopherbot