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: redundant loads in strconv not eliminated #59478

Open
nngffhj opened this issue Apr 7, 2023 · 3 comments
Open

cmd/compile: redundant loads in strconv not eliminated #59478

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

Comments

@nngffhj
Copy link

nngffhj commented Apr 7, 2023

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

$ go version
go version go1.20.3 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/czhong4/.cache/go-build"
GOENV="/home/czhong4/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/czhong4/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/czhong4/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/czhong4/gocode/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/czhong4/gocode/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/czhong4/gocode/go/src/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build822587075=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I ran a redundant loads detecting tool on function strconv.FormatFloat().

What did you expect to see?

Redundant loads can be optimized by the compiler.

What did you see instead?

  1. In function rightShift() in package strconv,
...
for ; n>>k == 0; r++ {
        if r >= a.nd {
...

a.nd is a loop invariant. But the compiler failed to put it in a register, resulting in a.nd being loaded from memory repeatedly in each iteration.

  1. In function Assign() in package strconv,
...
a.nd = 0
for n--; n >= 0; n-- {
	a.d[a.nd] = buf[n]
	a.nd++
}
...

a.nd is loaded from memory twice in a single iteration.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 7, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Apr 7, 2023

CC @golang/compiler

CC @dr2chase in particular, who I think was recently looking into loop invariant hoisting. I think there's an open bug for loop invariant hoisting already, and IIUC (though I could be wrong) that optimization doesn't make a meaningful difference in real-world programs, only microbenchmarks. It might still be worth doing as it adds up, but I just wanted to add that context.

@mknyszek mknyszek added this to the Backlog milestone Apr 7, 2023
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 7, 2023
@randall77
Copy link
Contributor 

a.nd = 0
for n--; n >= 0; n-- {
	a.d[a.nd] = buf[n]
	a.nd++
}

Hoisting the a.nd loads is tricky because the compiler currently doesn't do any alias analysis, so the write to a.d could be one of the bytes of a.nd. This is on the pretty easy end of alias analysis, though, as the same base pointer makes it pretty obvious. So maybe doable.

Some loop rotation might get you down to 1 load of a.nd per iteration.

@mknyszek mknyszek modified the milestones: Backlog, Unplanned Apr 12, 2023
@mknyszek mknyszek changed the title cmd/compile: Inefficiencies in binary code cmd/compile: redundant loads in strconv not eliminated Apr 12, 2023
@mknyszek
Copy link
Contributor

@randall77 Is there another issue out for this? I feel like we already have a redundant loads issue somewhere, though maybe this specific one is different.

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. FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mknyszek @bcmills @randall77 @gopherbot @nngffhj