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: suboptimal bits.Rotate* on arm64 #48465

Closed
greatroar opened this issue Sep 19, 2021 · 1 comment
Closed

cmd/compile: suboptimal bits.Rotate* on arm64 #48465

greatroar opened this issue Sep 19, 2021 · 1 comment
Labels
arch-arm64 FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance

Comments

@greatroar
Copy link

greatroar commented Sep 19, 2021

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

$ go version
go version go1.17.1 linux/amd64

Does this issue reproduce with the latest release?

Reproduces with

$ gotip version
go version devel go1.18-771b8ea4f4c Sun Sep 19 02:43:09 2021 +0000 linux/amd64

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

GOARCH=arm64

What did you do?

func rotxor1(x, y uint64) uint64 {
	y = (y << 7) | (y >> 57)
	return x ^ y
}

func rotxor2(x, y uint64) uint64 {
	return x ^ bits.RotateLeft64(y, 7)
}

What did you expect to see?

Would have been nice to see a fused rotate-xor (#48002), but at least the generated asm should be the same.

What did you see instead?

For rotxor1:

MOVD	"".y+8(FP), R0
ROR	$57, R0, R0
MOVD	"".x(FP), R1
EOR	R0, R1, R0

For rotxor2:

MOVD	"".y+8(FP), R0
MOVD	$-7, R1
ROR	R1, R0, R0
MOVD	"".x(FP), R1
EOR	R0, R1, R0

The compiler inserts a mov and allocates a register for the shift constant when the intrinsic is used. This is a regression introduced in Go 1.12. It's not limited to these tiny functions: I spotted this when inspecting the generated asm for SipHash.

@greatroar greatroar changed the title Suboptimal rotate+xor on arm64, worse with bits.Rotate* Suboptimal bits.Rotate* on arm64 Sep 19, 2021
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2021
@ALTree ALTree changed the title Suboptimal bits.Rotate* on arm64 cmd/compile: suboptimal bits.Rotate* on arm64 Sep 19, 2021
@gopherbot
Copy link

Change https://golang.org/cl/350909 mentions this issue: cmd/compile: implement constant rotates on arm64

@golang golang locked and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

3 participants