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: small struct initialization code is suboptimal because of redundant zeroing #59021

Closed
tdakkota opened this issue Mar 14, 2023 · 2 comments
Closed

cmd/compile: small struct initialization code is suboptimal because of redundant zeroing #59021

tdakkota opened this issue Mar 14, 2023 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@tdakkota
Copy link

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

$ go version
go version go1.20.1 windows/amd64

Also tested: go1.21-f5c7416511, go1.19

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\tdakkota\AppData\Local\go-build
set GOENV=C:\Users\tdakkota\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\tdakkota\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\tdakkota\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=G:\scoop\apps\go\current
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=G:\scoop\apps\go\current\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\tdakkota\Desktop\tmp\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config

What did you do?

https://go.dev/play/p/KYZ1WC9j4VP
https://go.godbolt.org/z/1KMEo5nP8 (note: uses Go 1.19)

What did you expect to see?

Compiler should generate the same code for new(T) and &T{}.

What did you see instead?

new(T) compiles to

        MOVUPS  X15, main..autotmp_1+8(SP)

but &T{} compiles to

        MOVUPS  X15, main..autotmp_2+8(SP) 
        MOVUPS  X15, main..autotmp_2+8(SP) // overwrite zeroed value with zero

Big structures like this

type T struct {
    A, B, C, D, E int
}

are not affected.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 14, 2023
@randall77
Copy link
Contributor

This is a failure of dead store elimination. It comes about because at that point in compilation, it doesn't realize that the two stores are actually to the same address, because they are different LocalAddr ops at different memory states.

v6 (8) = LocalAddr <*T> {.autotmp_2} v2 v1
v3 (8) = OffPtr <*int> [8] v6
v22 (8) = OffPtr <*int> [0] v6
v4 (8) = Store <mem> {int} v22 v5 v1
v26 (8) = Store <mem> {int} v3 v5 v4
v8 (8) = LocalAddr <*T> {.autotmp_2} v2 v26 (a[*T])
v23 (8) = OffPtr <*int> [8] v8
v25 (8) = OffPtr <*int> [0] v8
v27 (+8) = Store <mem> {int} v25 v5 v26
v28 (8) = Store <mem> {int} v23 v5 v27

We could teach DSE that two LocalAddrs of the same variable are equal, even if they are from different memory states.

We could also get rid of the memory dependence of LocalAddr for types with no pointers in them. We really only need to be that picky when there is a GC implication of seeing a pointer early. (And even then, with conservative top frame scanning, we only need to worry about early pointers across calls.)

@randall77 randall77 added this to the Unplanned milestone Mar 14, 2023
@cherrymui cherrymui modified the milestones: Unplanned, Backlog Mar 14, 2023
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 14, 2023
@cherrymui cherrymui modified the milestones: Backlog, Unplanned Mar 14, 2023
derekparker added a commit to derekparker/go that referenced this issue Apr 11, 2024
@derekparker
cmd/compile: teach dse about equivalent LocalAddrs
ebdbd87 
Fixes golang#59021
derekparker added a commit to derekparker/go that referenced this issue Apr 11, 2024
@derekparker
cmd/compile: teach dse about equivalent LocalAddrs
45eb69e 
Fixes golang#59021
derekparker added a commit to derekparker/go that referenced this issue Apr 11, 2024
@derekparker
cmd/compile: teach dse about equivalent LocalAddrs
bc09336 
Fixes golang#59021
derekparker added a commit to derekparker/go that referenced this issue Apr 11, 2024
@derekparker
cmd/compile: teach dse about equivalent LocalAddrs
e15c9f7 
Fixes golang#59021
@derekparker derekparker mentioned this issue Apr 11, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/578376 mentions this issue: cmd/compile: teach dse about equivalent LocalAddrs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

cmd/compile: teach dse about equivalent LocalAddrs derekparker/go
4 participants
@randall77 @gopherbot @cherrymui @tdakkota