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.

Sign up for GitHub

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/asm: noise and bad assembly for constants on arm64 #16226

Closed
aclements opened this issue Jun 30, 2016 · 7 comments
Closed

cmd/asm: noise and bad assembly for constants on arm64 #16226

aclements opened this issue Jun 30, 2016 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@aclements
Copy link
Member

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

go version go1.6.2 linux/amd64 (also happens at tip, 92ce6c20)

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

GOARCH="arm64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/austin/r/go"
GORACE=""
GOROOT="/home/austin/.cache/gover/1.6.2"
GOTOOLDIR="/home/austin/.cache/gover/1.6.2/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -fmessage-length=0"
CXX="g++"
CGO_ENABLED="0"

What did you do?

$ echo "package main" > x.go
$ cat >x.s <<EOF
TEXT main·main(SB), 0, $0-0
        ORR $0x100000000, R0
        ORR $0x10000, R0
        ORR $0x100, R0
        ORR $0x1, R0
        RET
EOF
$ GOARCH=arm64 go build

What did you expect to see?

I expected it to assembly silently and produce four ORR instructions using shifted immediates (maybe the first one would need a constant from the constant pool).

What did you see instead?

The assembler prints the following messages:

addpool: ADDCON0 in 00032 (/home/austin/tmp/test/x.s:4) ORR $256, R0 shouldn't go to default case
addpool: ADDCON0 in 00040 (/home/austin/tmp/test/x.s:5) ORR $1, R0 shouldn't go to default case

Despite these messages, the build succeeds. However, all four ORRs are compiled to first load the constant from the constant pool, even though most of them could clearly be encoded as immediate ORRs.

Worse, the first ORR is compiled to use a 32-bit constant, which isn't even big enough for the constant, so it winds up ORR'ing 0.

This can be clearly seen in the objdump output:

0000000000011090 <main.main>:
   11090:       f9400b81        ldr     x1, [x28,#16]
   11094:       910003e2        mov     x2, sp
   11098:       eb01005f        cmp     x2, x1
   1109c:       54000149        b.ls    110c4 <main.main+0x34>
   110a0:       180001bb        ldr     w27, 110d4 <main.main+0x44>
   110a4:       aa1b0000        orr     x0, x0, x27
   110a8:       1800019b        ldr     w27, 110d8 <main.main+0x48>
   110ac:       aa1b0000        orr     x0, x0, x27
   110b0:       1800017b        ldr     w27, 110dc <main.main+0x4c>
   110b4:       aa1b0000        orr     x0, x0, x27
   110b8:       1800015b        ldr     w27, 110e0 <main.main+0x50>
   110bc:       aa1b0000        orr     x0, x0, x27
   110c0:       d65f03c0        ret
   110c4:       aa1e03e3        mov     x3, x30
   110c8:       9401421a        bl      61930 <runtime.morestack_noctxt>
   110cc:       17fffff1        b       11090 <main.main>
   110d0:       14000000        b       110d0 <main.main+0x40>
   110d4:       00000000        .inst   0x00000000 ; undefined
   110d8:       00010000        .inst   0x00010000 ; undefined
   110dc:       00000100        .inst   0x00000100 ; undefined
   110e0:       00000001        .inst   0x00000001 ; undefined
@aclements aclements added this to the Go1.8 milestone Jun 30, 2016
@robpike
Copy link
Contributor

robpike commented Jun 30, 2016

I observe that in cmd/internal/obj/arm64/asm7.go the line that prints that message is preceded just above by the comment // TODO(aram): remove.

@aclements
Copy link
Member Author

Sure, though the message itself is arguably legitimate. We shouldn't even be in that code for the smaller constants.

@gopherbot
Copy link

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

@josharian
Copy link
Contributor

@4ad

@mwhudson
Copy link
Contributor

mwhudson commented Jul 1, 2016

I think this is down to arm64 bitwise immediates being encoded in an unusual scheme that Go's backend does not attempt to handle at all, which is #10111. I have a start of some code to address this in https://github.com/mwhudson/go/tree/bimm but I'm not super excited to finish it...

@4ad
Copy link
Member

4ad commented Jul 1, 2016 •
edited

Yes.

Constant encoding on arm64 is serious bussiness. There are many different ways you can encode constant loads on arm64. Some of them survived several attempts of debugging, so they were disabled, leading to this observed behavior.

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Aug 16, 2017
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.8
Development

No branches or pull requests
7 participants
@josharian @mwhudson @4ad @aclements @robpike @gopherbot @cherrymui