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: unnecessary bounds check in a loop passed as a closure #39756

Open
jordanlewis opened this issue Jun 22, 2020 · 1 comment
Open

cmd/compile: unnecessary bounds check in a loop passed as a closure #39756

jordanlewis opened this issue Jun 22, 2020 · 1 comment

Comments

@jordanlewis
Copy link

@jordanlewis jordanlewis commented Jun 22, 2020

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

$ go version

go version go1.13.10 darwin/amd64

Does this issue reproduce with the latest release?

Yes, using amd64 gc (tip) on go.godbolt.org.

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

go env Output
$ go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jordan/Library/Caches/go-build"
GOENV="/Users/jordan/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jordan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go@1.13/1.13.10_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go@1.13/1.13.10_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/var/folders/kz/_525lwtd4f7cxsb_qzwp0mv00000gn/T/go-build573559598=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I loaded the following program into go.godbolt.org:

package foo

func run(f func ()) {
   f()
}

func foo(c []int64, n int, v int64)  {
   run(func() {
       _ = c[n-1]
       for i := 0; i < n; i++ {
           c[i] = v
       }
   })
}

https://go.godbolt.org/z/iPaz7f

What did you expect to see?

I expected that the bounds check for c[i] would be eliminated because of the early bounds check triggered by c[n-1], the way that it would if the inner loop wasn't sent as a closure to run.

This should be possible because c doesn't get assigned to within the closure.

What did you see instead?

The compiler emits a bounds check for c[i].

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 22, 2020

@brtzsnr @rasky @zdjones

It's not just the bounds check, it is also reloading the backing store pointer every time. c is somehow being treated as having its address taken. I don't understand why that is.

You can fix this by doing

   run(func() {
       a := c
       _ = a[n-1]
       for i := 0; i < n; i++ {
           a[i] = v
       }
   })

a := c[:n] also works.

@randall77 randall77 added this to the Unplanned milestone Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.