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 doesn't combine simple chain #72832

Open
dominikh opened this issue Mar 13, 2025 · 7 comments
Open

cmd/compile: memcombine doesn't combine simple chain #72832

dominikh opened this issue Mar 13, 2025 · 7 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@dominikh
Copy link
Member

dominikh commented Mar 13, 2025 •
edited
Loading

Go version

go version go1.24.1 linux/amd64

Output of go env in your module/workspace:

-

What did you do?

go build -gcflags=-S

package pkg

var foo1 tile1
var foo2 tile2
var foo3 tile3

type tile1 struct {
	a uint16
	b uint16
	c uint32
}

type tile2 struct {
	a uint16
	b uint16
	c uint16
	d uint16
}

type tile3 struct {
	a uint16
	b uint16
	c uint8
	d uint8
	e uint16
}

func (a *tile1) store() { foo1 = *a }
func (a *tile2) store() { foo2 = *a }
func (a *tile3) store() { foo3 = *a }

What did you see happen?

tile2.store and tile3.store compile to MOVQ, as expected. tile1.store, however, uses two MOVL instead.

command-line-arguments.(*tile1).store STEXT nosplit size=18 args=0x8 locals=0x0 funcid=0x0 align=0x0
        0x0000 00000 (bar.go:28)     MOVL    (AX), CX
        0x0002 00002 (bar.go:28)     MOVL    4(AX), AX
        0x0005 00005 (bar.go:28)     MOVL    CX, command-line-arguments.foo1(SB)
        0x000b 00011 (bar.go:28)     MOVL    AX, command-line-arguments.foo1+4(SB)
        0x0011 00017 (bar.go:28)     RET

command-line-arguments.(*tile2).store STEXT nosplit size=11 args=0x8 locals=0x0 funcid=0x0 align=0x0
        0x0000 00000 (bar.go:29)     MOVQ    (AX), AX
        0x0003 00003 (bar.go:29)     MOVQ    AX, command-line-arguments.foo2(SB)
        0x000a 00010 (bar.go:29)     RET

command-line-arguments.(*tile3).store STEXT nosplit size=11 args=0x8 locals=0x0 funcid=0x0 align=0x0
        0x0000 00000 (bar.go:30)     MOVQ    (AX), AX
        0x0003 00003 (bar.go:30)     MOVQ    AX, command-line-arguments.foo3(SB)
        0x000a 00010 (bar.go:30)     RET

What did you expect to see?

I expected all 3 methods to compile the same, using MOVQ.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 13, 2025
@gabyhelp
Copy link

Related Issues

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

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Mar 13, 2025
@dr2chase
Copy link
Contributor

@golang/compiler

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 14, 2025
@randall77
Copy link
Contributor

memcombine is designed to handle patterns generated by encoding/binary operations, it is not (currently) expected to handle everything.

That said, it handles the tile2 case. tile3 is handled by a different mechanism entirely, as it takes the large struct path. That uses a whole-struct move instead of field-by-field moves.

tile1 fails because we process back to front, and at the start we can't find a pair for the uint32. We then later figure out that we can pair the two uint16s into a uint32. But we never go back and try again to pair the first uint32. Maybe a retry when some rewriting happens is all it will take to handle this issue. CLs welcome.

@randall77
Copy link
Contributor

The other option is to implement the TODO described here:

go/src/cmd/compile/internal/ssa/memcombine.go

Line 504 in 580b6ee

// TODO: the constant source and consecutive load source cases

@dominikh
Copy link
Member Author

How come that the mechanism used for tile3 doesn't work for tile1? They're both the same size, after all.

@randall77
Copy link
Contributor

The current rule to move to the large-struct regime is > 4 fields or > 4 words of memory.

@mknyszek mknyszek added this to the Unplanned milestone Mar 19, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/661855 mentions this issue: cmd/compile: run memcombine until converge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@dominikh @mknyszek @dr2chase @randall77 @gopherbot @cherrymui @gabyhelp