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: use mapaccess1_fast64 for small keys #66446

Open
dsnet opened this issue Mar 21, 2024 · 6 comments
Open

cmd/compile: use mapaccess1_fast64 for small keys #66446

dsnet opened this issue Mar 21, 2024 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Mar 21, 2024 •
edited

Go version

go1.22

Output of go env in your module/workspace:

GOARCH=amd64
GOOS=linux

What did you do?

Compile the following program:

package main

type key struct {
	u16 uint16
	u8  bool
	u32 uint32
}

var map1 map[key]struct{}
var map2 map[uint64]struct{}
var key1 = key{}
var key2 = uint64(0)
var sink struct{}

func main() {
	sink = map1[key1]
	sink = map2[key2]
}

What did you see happen?

The compiler output:

0x0023 00035 (main.go:17)	CALL	runtime.mapaccess1(SB)
...
0x0040 00064 (main.go:18)	CALL	runtime.mapaccess1_fast64(SB)

What did you expect to see?

runtime.mapaccess1_fast64 being used on both since the key type is smaller than a uint64.

That said, there is some complications. For example, through unsafe, someone could writes values to the padding in-between the bool field and the uint32. Also, someone using unsafe could write a bit pattern to the bool that isn't 0 or 1.
It isn't clear to me that this is something that someone can rely on.

However, even if we make the key struct exactly 64-bits wide with no padding and no bools, it still doesn't use runtime.mapaccess1_fast64.
@dsnet
Copy link
Member Author

dsnet commented Mar 21, 2024

\cc @randall77, maybe related to #66413?

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 21, 2024
@dsnet dsnet mentioned this issue Mar 21, 2024
Merged
@randall77
Copy link
Contributor

Not directly related, no.

We're pretty careful about not including data in padding as part of equality (and thus hash). I don't know if anyone really takes advantage of that, but I'm not sure we would want to change that at this point. That assumption is probably baked in in a few places (e.g. code that initializes stack variables might not zero those fields currently).

However, even if we make the key struct exactly 64-bits wide with no padding and no bools, it still doesn't use runtime.mapaccess1_fast64.

It does for me. Even with bools. Only padding causes problems. Could you double-check and post a repro if you have one?

@dsnet
Copy link
Member Author

dsnet commented Mar 21, 2024

Could you double-check and post a repro if you have one?

Ah, your right. I was testing with:

type key struct {
    a uint8
    b uint16
    c uint32
    d uint8
}

While that adds up to 64-bits, I had forgotten about alignment.

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 26, 2024
@mdempsky mdempsky added this to the Backlog milestone Mar 26, 2024
@mdempsky
Copy link
Member

Since mapaccess1_fast64 passes the key by register, and we know where the padding holes are in the type even when shaping is applied, so we could always just load the value and zero any padding bytes with a mask before calling into the runtime.

@randall77
Copy link
Contributor

Sure, that's a good idea.

@randall77
Copy link
Contributor

One complication: mapacess1_fast64 uses uint64 == to compare keys. so not only does mapacess1_fast64 need to do the masking, but inserts do as well.

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. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
4 participants
@mdempsky @dsnet @randall77 @gopherbot