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: maps doesn't recognize map-clearing range idiom after be inlined #53157

Open
cwbhhjl opened this issue May 31, 2022 · 1 comment
Open
Labels
NeedsInvestigation Performance
Milestone

Comments

@cwbhhjl
Copy link

@cwbhhjl cwbhhjl commented May 31, 2022

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

$ go version 1.18.2

Does this issue reproduce with the latest release?

yes

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

linux & windows , amd64

What did you do?

https://go.dev/play/p/wi_i3Dx8Hpa

package main

func clearMap(m map[string]string) {
	for k := range m {
		delete(m, k)
	}
}

//go:noinline
func clearMap2(m map[string]string) {
	for k := range m {
		delete(m, k)
	}
}

var m map[string]string

func main() {
	clearMap(m)
	clearMap2(m)
}

In go1.18, the compiler will inline clearMap by default, then generates:

CALL runtime.mapiterinit(SB)
JMP 0x45b3c9
MOVQ 0(DX), CX
MOVQ 0x8(DX), DI
LEAQ type.*+23744(SB), AX
MOVQ 0x20(SP), BX
CALL runtime.mapdelete_faststr(SB)
LEAQ 0x28(SP), AX
CALL runtime.mapiternext(SB)
MOVQ 0x28(SP), DX
TESTQ DX, DX
JNE 0x45b3a7

and clearMap2 will call runtime.mapclear.

Is this expected behavior?

(go1.17 will not inline clearMap)

@randall77
Copy link
Contributor

@randall77 randall77 commented May 31, 2022

Yes, this is unfortunate. Due to how we do inlining, declarations that were formally local to the for loop in the inlined body are moved up to the top of the containing function. That loses the association between k and the for loop. We need that association because that's how the optimization knows that k is not used after the loop.

It would be nice to fix this, but I don't think it is easy.

@randall77 randall77 added this to the Unplanned milestone May 31, 2022
@dmitshur dmitshur added the NeedsInvestigation label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Performance
Projects
None yet
Development

No branches or pull requests

3 participants