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/internal/obj/arm64: golang assembler handles arm64 instruction madd/msub incorrectly. #20723

Closed
zhangfannie opened this issue Jun 19, 2017 · 9 comments

Comments

@zhangfannie
Copy link
Contributor

zhangfannie commented Jun 19, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version devel +59051da Fri Jun 16 06:05:57 2017 +0000 linux/arm64

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

GOARCH="arm64"
GOBIN=""
GOEXE=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fanzha02/go"
GORACE=""
GOROOT="/home/fanzha02/work/asm_test/golang_us/golang_1/golang"
GOTOOLDIR="/home/fanzha02/work/asm_test/golang_us/golang_1/golang/pkg/tool/linux_arm64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build468264647=/tmp/go-build -gno-record-gcc-switches"
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"

What did you do?

cd src/cmd/asm/internal/asm
add arm64enc.s test file
enable the ARM64Encoder test command
go test -run "ARM64Encoder"

arm64enc.s test file and endtoend_test.go you can load from this patch:
https://go-review.googlesource.com/c/45851/

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

go test successfully

What did you see instead?

the madd/maddw/msub/msubw test cases are failed

@gopherbot
Copy link

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

@ALTree
Copy link
Member

ALTree commented Jun 19, 2017

Was this in go1.8 too? Or it was handled correctly in go1.8 and the issue is new on tip (so, a recent regression)?

@zhangfannie
Copy link
Contributor Author

I just load the latest golang source code and build using go1.6 for bootstrapping. Then run the test and failed.
I also load go1.8 and run the test. And it shows the error infomation "package cmd/internal/objabi: cannot find package "cmd/internal/objabi"" .

@ALTree
Copy link
Member

ALTree commented Jun 19, 2017

Looking at the git history it looks like it was encoded like that for at least 2 years, so I'll guess that no, it's not a recent regression.

Leaving for others to decide if we want this in go1.9.

@cherrymui
Copy link
Member

Ok, I looked into this. The tests that @zhangfannie added is expecting MADD R1, R2, R3, R4 to mean R4 = R1*R2 + R3, but currently it means R4 = R1*R3 + R2.

I agree that the first form looks nicer. But this is potentially a breaking change. There may be user code that works that way, especially given that this has been like that for several releases. We need to be sure that no user will be affected unexpectedly, if we want to make this change.

On the other hand, on PPC64, similar FMADD instruction means /* FMADDx fra,frb,frc,frt (t=a*c±b) */. So maybe we shouldn't make this change.

@zhangfannie
Copy link
Contributor Author

zhangfannie commented Jun 20, 2017

@cherrymui I found the comment ("madd/msub Rm,Rn,Ra,Rd ") in the current code, the current code gets "Rn" value from prog.From3.Reg and gets "Ra" value from prog.Reg. Refer to the instructions assemble process(asm.go), if the comment is right, get arguments value in that way is wrong. And the comment means R4 = R1 * R3 + R2.
If the comment is wrong, the current code means R4 = R1 * R2+R3.
The fix considers the comment is right.

@cherrymui
Copy link
Member

One possibility is to fix the comment to match the implementation. I'm not sure.

@zhangfannie
Copy link
Contributor Author

zhangfannie commented Jun 21, 2017

@cherrymui @ALTree I will fix the comment and keep the source code not changed.
Please refer to the fix patch:https://go-review.googlesource.com/c/45850/9

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 30, 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

4 participants