cmd/compile: redundant loads in strconv not eliminated

nngffhj commented 1 year ago

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?

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.
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.

mknyszek commented 1 year ago

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.

randall77 commented 1 year ago

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 commented 1 year ago

@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.

golang / go