-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: closure (non-inlined code in general) integer conversions broken in go1.10beta1 #23305
Comments
In case this is helpful or relevant it appears adding |
There have been some commits in the 1.10 window to teach the compiler to inline certain closures - see the commits from late 2017 in https://github.com/golang/go/commits?author=huguesb. Not certain that those are the cause as I haven't checked, but they do seem good candidates. /cc @huguesb @randall77 @mdempsky |
@mvdan note that the second playground example, which adds For clarity, here is just that example, identical functions with pragma noinline, and without that pragma, producing different results against go1.10beta1 (amd64/linux). https://play.golang.org/p/p40iIhUIWcP ... I guess I'm saying that closures may have been a red-herring here, but because in general they do not inline, this may be why I was seeing it with a closure, the problem looks more generic. |
@mvdan Looking at the play link it seems unlikely that my commits are related: closures can currently only be inlined when they're invoked inside the same function where they are defined, and functions that define a closure cannot be inlined. In other words, the closure part of this example should be compiled identically with or without my changes. |
I stand corrected then - when I read closures and inlining, those commits were the first thing that came to mind. |
Simplified repro case ( https://play.golang.org/p/uDjsdv8MCTQ ):
|
Simpler repro: https://play.golang.org/p/W75JlEAUlaT The issue appears to be in SSA lowering. I see these SSA ops:
get lowered into:
It looks like the zero-extend op is getting lost. /cc @randall77 @cherrymui |
As far as I can tell, the normal function is misbehaving. It happens to work when inlined though. |
The problem is this rule. In
it assumes a Select Op has upper bit zeroed, which seems not true. |
In theory, Introduced from https://go-review.googlesource.com/c/go/+/58090. |
Change https://golang.org/cl/85736 mentions this issue: |
Do these conversions get any test coverage in the standard testing suite? |
Looks like a problem with DIVL lowering
Is lowered into:
|
I've just checked and generating NEGL instead of NEGQ for DIVL also fixes this issue. |
Reopening for 1.10 cherry pick. |
@mdempsky, no need for re-open. We don't branch Go 1.10 until the rc1. It's not there yet: https://go.googlesource.com/go/+refs |
Closing for no 1.10 cherry pick. |
Should we open a new issue or queue up a CL for 1.11 to re-enable and fix the optimization? |
@aclements Done: #23310. |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?go version devel +9ce6b5c Thu Dec 7 17:38:51 2017 +0000 linux/amd64
a.k.a "go1.10beta1"
Does this issue reproduce with the latest release?
No, it can not be reproduced on go1.9 release.
What operating system and processor architecture are you using (
go env
)?GOARCH="amd64"
GOBIN="/work/go/go/bin/"
GOCACHE="/home/user/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/work/user/go"
GORACE=""
GOROOT="/work/go/go"
GOTMPDIR=""
GOTOOLDIR="/work/go/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build975595394=/tmp/go-build -gno-record-gcc-switches"
What did you do?
Ran the following (abstracted/extracted from my production code).
https://play.golang.org/p/jBT07pKiFE2
What did you expect to see?
I expected to see the identical functions, generate identical results, and so the example linked should print nothing.
In particular I expect an int32 value cast to uint32 cast to uint64, to produce an output that is representable in 32b, with the top 32b always 0.
What did you see instead?
What I see instead is that the function inside the closure produces an extended result; it appears the that something happens to the conversion --> uint64(uint32(int32)). I end up with a result where the top 32b are not zero, which looks broken to me, with reference to the language spec discussion on conversion.
In anycase I'd expect both the closure-generated function (via generator L17) and the function mask (L7) to produce identical results.
The text was updated successfully, but these errors were encountered: