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: memcombine does not combine stores separated by OpLocalAddr #70300

Closed
fengyoulin opened this issue Nov 12, 2024 · 8 comments
Closed

cmd/compile: memcombine does not combine stores separated by OpLocalAddr #70300

fengyoulin opened this issue Nov 12, 2024 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@fengyoulin
Copy link
Contributor

Go version

devel go1.24-c96939fbed

Output of go env in your module/workspace:

GOARCH='amd64'

What did you do?

Check the assembly code of this function:

func foo(v uint64) (b [8]byte) {
	b[0] = byte(v)
	b[1] = byte(v >> 8)
	b[2] = byte(v >> 16)
	b[3] = byte(v >> 24)
	b[4] = byte(v >> 32)
	b[5] = byte(v >> 40)
	b[6] = byte(v >> 48)
	b[7] = byte(v >> 56)
	return b
}

What did you see happen?

88442408                MOVB AL, 0x8(SP)
4889c1                  MOVQ AX, CX
48c1e808                SHRQ $0x8, AX
88442409                MOVB AL, 0x9(SP)
4889c8                  MOVQ CX, AX
48c1e910                SHRQ $0x10, CX
884c240a                MOVB CL, 0xa(SP)
4889c1                  MOVQ AX, CX
48c1e818                SHRQ $0x18, AX
8844240b                MOVB AL, 0xb(SP)
4889c8                  MOVQ CX, AX
48c1e920                SHRQ $0x20, CX
884c240c                MOVB CL, 0xc(SP)
4889c1                  MOVQ AX, CX
48c1e828                SHRQ $0x28, AX
8844240d                MOVB AL, 0xd(SP)
4889c8                  MOVQ CX, AX
48c1e930                SHRQ $0x30, CX
884c240e                MOVB CL, 0xe(SP)
48c1e838                SHRQ $0x38, AX
8844240f                MOVB AL, 0xf(SP)
c3                      RET

What did you expect to see?

4889442408              MOVQ AX, 0x8(SP)
c3                      RET
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 12, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/627255 mentions this issue: cmd/compile: teach combineStores not to be confused by OpLocalAddr between OpStores

@fengyoulin fengyoulin changed the title cmd/compile: memcombine confused by OpLocalAddr between OpStores cmd/compile: memcombine does not combine stores separated by OpLocalAddr Nov 13, 2024
@mknyszek mknyszek added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 13, 2024
@mknyszek mknyszek added this to the Unplanned milestone Nov 13, 2024
@randall77
Copy link
Contributor

I feel like the right solution here is to not issue so many OpLocalAddr in the first place.
We're pretty conservative about how we generate them, but especially as the target of the reference has no pointers, we really don't have to be super careful.
I'm thinking that in ssagen we can use a single OpLocalAddr (like we do for PPARAM variables) for any PPARAMOUT variables that don't have pointers in them. Or something like that.
We could so CSE the OpLocalAddr, perhaps? If one dominates the other we can use the first one.

@fengyoulin
Copy link
Contributor Author

It sounds reasonable, but there is another thing I don't quite understand, since Ops like OpStore and OpLoad have their own memory args, why does OpLocalAddr also need a memory arg?

@fengyoulin
Copy link
Contributor Author

I found the explanation in CL 122483 which introduced OpLocalAddr. I am studying it.

@randall77
Copy link
Contributor

https://go-review.googlesource.com/c/go/+/122484 (which looks abandoned-ish @dr2chase) implements the CSE part I suggested. My other suggestions are in there as comments as well.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/628075 mentions this issue: cmd/compile: modify CSE to remove redundant OpLocalAddrs

@fengyoulin
Copy link
Contributor Author

Change https://go.dev/cl/628075 mentions this issue: cmd/compile: modify CSE to remove redundant OpLocalAddrs

This CL is ready for review. PTAL. @randall77

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Nov 22, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 22, 2024
This was referenced Feb 16, 2025
Open
Open
@gabyhelp gabyhelp mentioned this issue Mar 13, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
Go Compiler / Runtime
Status: Done
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@mknyszek @dmitshur @randall77 @gopherbot @fengyoulin @gabyhelp