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: inlining function that references function literals generates bad code #54632

Closed
dr2chase opened this issue Aug 23, 2022 · 6 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dr2chase
Copy link
Contributor

dr2chase commented Aug 23, 2022

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

go version devel go1.20-e7e939ba0d Tue Aug 23 11:02:34 2022 -0400 darwin/amd64$ go version

Does this issue reproduce with the latest release?

No, unless GOEXPERIMENT=unified is enabled. This is linked to unified IR.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/drchase/Library/Caches/go-build"
GOENV="/Users/drchase/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/drchase/work/gocode/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/drchase/work/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/drchase/work/go-unified"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/drchase/work/go-unified/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel go1.20-e7e939ba0d Tue Aug 23 11:02:34 2022 -0400"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v6/9yfw749x623dr55rpm6ntnsc0095tn/T/go-build2628182485=/tmp/go-build -gno-record-gcc-switches -fno-common"
(also observed on Linux)

What did you do?

Ran code calling two identical functions, one inlined, the other not

What did you expect to see?

Identical results (and all "PASS")

go run bug.go
PASS
PASS
PASS

What did you see instead?

Different results (some "FAIL")

go run bug.go
PASS
FAIL
FAIL
@dr2chase
Copy link
Contributor Author

dr2chase commented Aug 23, 2022

@mdempsky

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 23, 2022
@dr2chase
Copy link
Contributor Author

dr2chase commented Aug 23, 2022

It depends upon inlining fail -- if that is //go:noinline it passes. I.e., there is a problem w/ constant propagating the function value.

@mdempsky mdempsky self-assigned this Aug 23, 2022
@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 23, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Aug 23, 2022
@dr2chase
Copy link
Contributor Author

dr2chase commented Aug 23, 2022

Here's a reproducer with a smaller AST: https://go.dev/play/p/FGmrix8m3nh?v=gotip

@mdempsky
Copy link
Member

mdempsky commented Aug 23, 2022

Thanks! There's something going on with the inliner and ir.reassigned.

During inlining of inlined(true) into main, we create new eff and isTrue local variables for main, and then generate a function body to use them, which can ultimately be substituted for the original function call in main.

But it seems like we're trying to recursively inline the resulting expanded body before it's been substituted into main. So when we called ir.reassigned(eff) to see whether it's been reassigned, we end up looking at the main, but we won't yet see any assignments to it. And thus the inliner decides it's safe to assume eff is never reassigned, and the call to eff() is always to fail.

I think the nounified frontend avoids this inliner bug, because it doesn't set Defn on local variables constructed during inlining, so we'd never inline calls in this case anyway.

Even further minimized repro: https://go.dev/play/p/jDtyqFOdFlB?v=gotip

@mdempsky
Copy link
Member

mdempsky commented Aug 23, 2022

Variation of the bug that affects Go 1.19: https://go.dev/play/p/Orb0U2fXXhB

@gopherbot
Copy link

gopherbot commented Aug 23, 2022

Change https://go.dev/cl/425257 mentions this issue: cmd/compile: defer transitive inlining until after AST is edited

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants