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

Jump to bottom

cmd/compile: maps doesn't recognize map-clearing range idiom after be inlined #53157

Closed
cwbhhjl opened this issue May 31, 2022 · 3 comments
Closed

cmd/compile: maps doesn't recognize map-clearing range idiom after be inlined #53157

cwbhhjl opened this issue May 31, 2022 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@cwbhhjl
Copy link

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

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 1, 2022
@randall77 randall77 mentioned this issue Jun 7, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@wdvxdr1123
Copy link
Contributor

I notice unified ir can fix this issue.

@randall77
Copy link
Contributor

Fixed in unified IR, which is now enabled by default.

@golang golang locked and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@dmitshur @cwbhhjl @randall77 @gopherbot @wdvxdr1123