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: test/fixedbugs/issue6036.go fails to compile for arm64 #18933

Closed
rahulchaudhry opened this issue Feb 4, 2017 · 7 comments
Closed
Milestone

Comments

@rahulchaudhry
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8rc3 linux/amd64

What did you do?

The test is in the go repo: "test/fixedbugs/issue6036.go"

$ GOARCH=arm64 go tool compile $GOROOT/test/fixedbugs/issue6036.go
test/fixedbugs/issue6036.go:45: negative large offset
00024 (test/fixedbugs/issue6036.go:20) MOVD 2147483648(R0), R1
test/fixedbugs/issue6036.go:45: negative large offset
00032 (test/fixedbugs/issue6036.go:20) MOVD R1, 2147483656(R0)
test/fixedbugs/issue6036.go:45: negative large offset
00028 (test/fixedbugs/issue6036.go:26) MOVB R1, 2147483649(R0)

The test compiles with go1.7.5, so this is a regression.

A quick bisect points to https://go-review.googlesource.com/#/c/26950: [dev.ssa] cmd/compile, etc.: more ARM64 optimizations, and enable SSA by default

@bradfitz bradfitz added this to the Go1.9 milestone Feb 4, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2017

Um, but the build dashboard is happy for the arm64 column:

https://build.golang.org/?branch=release-branch.go1.8

... which runs that code presumably?

Oh, that file is // +build amd64.

Not sure why.

/cc @randall77 @cherrymui @minux @josharian

@ianlancetaylor
Copy link
Member

The test won't build on 32-bit targets, but should ideally be compiled on all 64-bit targets.

Probably the //build line should be written as a negative constraint which will be easy to find and modify for any new 32-bit target.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/36318 mentions this issue.

@cherrymui
Copy link
Member

Are we targeting this for Go 1.8? This is a regression due to SSA beckend.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 6, 2017

@cherrymui, your call. You comfortable with it (safe enough?) and worth it? Does this impact real code?

@cherrymui
Copy link
Member

Ok. It only makes optimization more conservative, so it should be safe. It would affect real code if the code does use such a huge array... Will send a CL for Go 1.8.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/36413 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 6, 2017
Fixes #18933.

Change-Id: I1ab524fdca006100ec6af572065b496f68d6a5c3
Reviewed-on: https://go-review.googlesource.com/36413
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants