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/compile: store combining doesn't combine all possible stores #60709

Closed
dr2chase opened this issue Jun 9, 2023 · 3 comments
Closed

cmd/compile: store combining doesn't combine all possible stores #60709

dr2chase opened this issue Jun 9, 2023 · 3 comments
Assignees
@randall77
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Go1.22

Comments

@dr2chase
Copy link
Contributor

dr2chase commented Jun 9, 2023

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

$ go version
go version devel go1.21-a0d053f626 Tue Jun 6 15:48:08 2023 -0400 darwin/amd64

Does this issue reproduce with the latest release?

yes, also in 1.17 (so it is not a regression)

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

The same bug was observed compiling for riscv64, arm64, and amd64

What did you do?

I compiled

package foobar

func baz(p, q *[4]uint8) {
	p[0], p[1], p[2], p[3] = q[0], q[1], q[2], q[3]
}

with GOOS=linux goarch=arm64 go build -gcflags=-S fb.go

What did you expect to see?

A single load-word (32 bits) instruction and a single store-word instruction.

What did you see instead?

Across those 3 architectures:

  • a 1-byte load from q[2]
  • a 2-byte load from q[0:2]
  • a 1-byte load q[3]

followed by stores in memory order. For example:

	0x0000 00000 (.../fb.go:4)	MOVBLZX	2(BX), CX
	0x0004 00004 (.../fb.go:4)	MOVWLZX	(BX), DX
	0x0007 00007 (.../fb.go:4)	MOVBLZX	3(BX), BX
	0x000b 00011 (.../fb.go:4)	MOVW	DX, (AX)
	0x000e 00014 (.../fb.go:4)	MOVB	CL, 2(AX)
	0x0011 00017 (.../fb.go:4)	MOVB	BL, 3(AX)
	0x0014 00020 (.../fb.go:5)	RET
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 9, 2023
@dr2chase dr2chase added this to the Backlog milestone Jun 9, 2023
@dr2chase
Copy link
Contributor Author

dr2chase commented Jun 9, 2023

@randall77 if you are interested, or perhaps this is a "help wanted".

@dr2chase dr2chase added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels Jun 9, 2023
@randall77
Copy link
Contributor

This is not really the pattern that the memcombine pass looks for. That requires the shift+or patterns that encoding/binary generates. The combining we do see here are from arch-specific rules.

Not sure why you wouldn't just do *p = *q here. But the main point stands, there are probably other similar patterns that don't have better phrasings.

@gopherbot
Copy link

Change https://go.dev/cl/502295 mentions this issue: cmd/compile: memcombine if values being stored are from consecutive loads

@mknyszek mknyszek modified the milestones: Backlog, Go1.22 Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@randall77 randall77

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
4 participants
@mknyszek @dr2chase @randall77 @gopherbot