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

Jump to bottom

cmd/compile: assembly print regression #21064

Closed
zhangfannie opened this issue Jul 18, 2017 · 14 comments
Closed

cmd/compile: assembly print regression #21064

zhangfannie opened this issue Jul 18, 2017 · 14 comments
Labels
FrozenDueToAge
Milestone
Go1.9

Comments

@zhangfannie
Copy link
Contributor

zhangfannie commented Jul 18, 2017 •
edited

Please answer these questions before submitting your issue. Thanks!

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

go version devel +0d482b3 Mon Jul 17 18:18:08 2017 +0000 linux/arm64

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

fanzha02@fanzha02-01-arm-vm:~/golangtmp$ ./bin/go env
GOARCH="arm64"
GOBIN=""
GOEXE=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fanzha02/go"
GORACE=""
GOROOT="/home/fanzha02/golangtmp"
GOTOOLDIR="/home/fanzha02/golangtmp/pkg/tool/linux_arm64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build506052363=/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?

  1. create a new go program and save as test.go file.
    package main func g(x *int) { *x = *x+2 } func f(x int) { g(&x) } func main(){ f(2) }
  2. $ /home/fanzha02/golangtmp/bin/go tool compile -S -l test.go > test,s
  3. please refer to the below assembly code.
    0x0000 00000 (test1.go:16) TEXT "".f(SB), $88-8
    0x0000 00000 (test1.go:16) MOVD 16(g), R1
    0x0004 00004 (test1.go:16) MOVD RSP, R2
    0x0008 00008 (test1.go:16) CMP R1, R2
    0x000c 00012 (test1.go:16) BLS 140
    0x0010 00016 (test1.go:16) MOVD.W R30, -96(RSP)
    0x0014 00020 (test1.go:16) FUNCDATA ZR, gclocals·263043c8f03e3241528dfae4e2812ef4(SB)
    0x0014 00020 (test1.go:16) FUNCDATA $1, gclocals·e226d4ae4a7cad8835311c6a4683c14f(SB)
    0x0014 00020 (test1.go:16) MOVD $"".x(FP), R0
    0x0018 00024 (test1.go:17) MOVD R0, 8(RSP)
    0x001c 00028 (test1.go:17) PCDATA ZR, ZR
    0x001c 00028 (test1.go:17) CALL "".g(SB)
    0x0020 00032 (test1.go:18) MOVD "".x(RSP), R0
    `
  4. when assembler assembles instruction "MOVD x(RSP), R0" and gives the error information "expected pseudo-register; found RSP".

I'd argue that two instructions "MOVD "".x(RSP), R0" and "MOVD $"".x(FP), R0" are misleading. In addition, the expression of "".x(RSP) and 8(RSP) are confusing . For go1.6, the paramenter's base register is printed as FP.

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?

What did you see instead?
@zhangfannie
Copy link
Contributor Author

zhangfannie commented Jul 18, 2017 •
edited

https://go-review.googlesource.com/c/37255/3 this patch modified the util.go

@cherrymui
Copy link
Member

Essentially dup of #19458. MOVD $"".x(FP), R0 should be SP though.

@cherrymui cherrymui changed the title cmd/assemble: ARM64 assembler regression issue. cmd/compile: ARM64 assembler regression issue Jul 18, 2017
@gopherbot
Copy link

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

@zhangfannie
Copy link
Contributor Author

@cherrymui Thank you for your response. I have checked the CL https://golang.org/cl/49432 and this patch fixes the issue that MOVD $"".x(FP), R0 should be SP. I also have a question to ask you, the question is the CL https://go-review.googlesource.com/c/37971 means "MOVD "".x(RSP), R0" is correct? So is it right that the paramenter's base register is printed as RSP? But why the go assembler cannot assemble MOVD x(RSP), R0 but MOVD x(FP), R0?

@odeke-em
Copy link
Member

@cherrymui and @zhangfannie, am I right to say that this is a regression from Go1.8?
/cc @randall77

@cherrymui
Copy link
Member

Taking a careful look, this is worse than I thought initially. For this program

func g(a int, b int) int {
        var x [24]byte // use some frame
        _ = x
        return b
}

On AMD64, it prints SP offsets:

	0x0028 00040 (b.go:4)	MOVQ	"".b+48(SP), AX
	0x002d 00045 (b.go:7)	MOVQ	AX, "".~r2+56(SP)

But on ARM64 (actually any non-x86), the offsets are actually relative to FP, but the base register is printed as SP:

	0x0010 00016 (b.go:4)	MOVD	"".b+8(RSP), R0
	0x0014 00020 (b.go:7)	MOVD	R0, "".~r2+16(RSP)

I initially thought they were SP offset but printed with FP base register. Maybe we want to change the offset to actual SP offset? Or we change back to FP on these architectures?
cc @randall77

@cherrymui
Copy link
Member

@odeke-em, I think so.

@zhangfannie
Copy link
Contributor Author

@cherrymui , I have also mentioned the worse thing in my comment that you found and you can review this patch: https://go-review.googlesource.com/c/37255. This patch changed the FP print as RSP. This patch used AMD64 instruction LEAQ as an example, I think the LEAQ issue maybe caused by compiler not by assembly print program.

@cherrymui
Copy link
Member

I updated the CL. That should fix it.

@odeke-em
Copy link
Member

Sorry for the quick interruption, but @cherrymui what milestone should we mark this issue for Go1.9, Go1.9Maybe or Go1.10? I ask because Go1.9rc1 was already cut.

@cherrymui
Copy link
Member

@odeke-em, I'd like to get this fixed in Go 1.9. I feel Go 1.9 milestone is ok. Others may correct me.

@odeke-em odeke-em added this to the Go1.9 milestone Jul 25, 2017
@zhangfannie
Copy link
Contributor Author

@cherrymui Could you please provide the CL that fixes it? Thank you.

@cherrymui cherrymui changed the title cmd/compile: ARM64 assembler regression issue cmd/compile: assembly print regression Jul 26, 2017
@cherrymui
Copy link
Member

Same CL, https://go-review.googlesource.com/c/49432/.

@gopherbot
Copy link

Change https://golang.org/cl/53130 mentions this issue: [release-branch.go1.9] cmd/compile: set/unset base register for better assembly print

@golang golang locked and limited conversation to collaborators Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
4 participants
@odeke-em @gopherbot @cherrymui @zhangfannie