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: assembly print regression #21064

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

Comments

Projects
None yet
4 participants
@zhangfannie
Copy link
Contributor

commented Jul 18, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

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

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

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

This comment has been minimized.

Copy link

commented Jul 18, 2017

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

@zhangfannie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

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

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

@odeke-em, I think so.

@zhangfannie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

I updated the CL. That should fix it.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

@gopherbot gopherbot closed this in f20944d Aug 2, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Aug 3, 2017

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.
You can’t perform that action at this time.