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 with type params doesn't recognize map-clearing range idiom #51699

Closed
wgrr opened this issue Mar 16, 2022 · 3 comments
Closed

cmd/compile: maps with type params doesn't recognize map-clearing range idiom #51699

wgrr opened this issue Mar 16, 2022 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@wgrr
Copy link
Contributor

wgrr commented Mar 16, 2022

In go1.18, the compiler output for this program (https://go.dev/play/p/GVufoDzAG5-) on tested targets {openbsd,linux}/{amd64,arm64,386} the generic version does not use runtime.mapclear, instead it iterates over the map deleting each element, I expected both functions generate a very similar code, instead clear generates (roughly):

mov    rbx,rax
lea    rax,type.map[string]bool
xchg   ax,ax
call   runtime.mapclear

clear2 (copied from exp/maps.Clear) generates (roughly):

call   runtime.mapiterinit
jmp    loop
start: mov rcx,QWORD PTR [rdx]
mov    rdi,QWORD PTR [rdx+0x8]
lea    rax,type.go.shape.map[string]bool_0
mov    rbx,QWORD PTR [rsp+0x20]
call   runtime.mapdelete_faststr
lea    rax,[rsp+0x28]
call   runtime.mapiternext
loop:  mov rdx,QWORD PTR [rsp+0x28]
test   rdx,rdx
jne    start
go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/w/bin"
GOCACHE="/home/w/.cache/go-build"
GOENV="/home/w/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/w/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/w/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="/usr/bin/gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build896486634=/tmp/go-build -gno-record-gcc-switches"
@randall77
Copy link
Contributor

In clear, at the point where we look for this optimization, we have:

.   .   DELETE-Args
.   .   .   NAME-main.m esc(no) Class:PPARAM Offset:0 OnStack MAP-map[string]bool tc(1) # issue51699.go:4:12
.   .   .   NAME-main.i esc(no) Class:PAUTO Offset:0 OnStack string tc(1) # issue51699.go:5:6

In clear2, we have

.   .   DELETE-Args
.   .   .   NAME-main.m esc(no) Class:PPARAM DictIndex:1 Offset:0 OnStack go.shape.map[string]bool_0 tc(1) # issue51699.go:11:46
.   .   .   CONVNOP Implicit string tc(1) # issue51699.go:13:9
.   .   .   .   NAME-main.i esc(no) Class:PAUTO DictIndex:2 Offset:0 OnStack go.shape.string_1 tc(1) # issue51699.go:12:6

The extra CONVNOP is because i can be anything of string shape, but the type of the map key must be exactly string. I think we can expand the mapclear optimization to allow nop conversions on the arguments.

That said, there may be a larger issue here, which is that I think the type of m should be map[go.shape.string_1]go.shape.bool_2, not go.shape.map[string]bool_0. Something about making shape types out of type parameters that are dependent on other type parameters might need some tweaking.

@cuonglm
Copy link
Member

cuonglm commented Mar 16, 2022

FYI, clear and clear2 are compiled the same with unified IR.

@gopherbot
Copy link

Change https://go.dev/cl/393434 mentions this issue: cmd/compile: allow noop conversions when comparing expressions

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 16, 2022
@heschi heschi added this to the Backlog milestone Mar 16, 2022
@golang golang locked and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@cuonglm @randall77 @gopherbot @wgrr @heschi