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: avoidable construction of stack objects #43753

Open
mdempsky opened this issue Jan 17, 2021 · 7 comments
Open

cmd/compile: avoidable construction of stack objects #43753

mdempsky opened this issue Jan 17, 2021 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@mdempsky
Copy link
Member

Compiling the program below with -live shows that x gets turned into a stack object:

package p

var m map[int16]bool

func f() {
        var x struct { p *int; i int16 }
        x.i = g()
        m[x.i] = true
        h()
        j(x.i)
}

func g() int16
func h()
func j(int16)

This is because the assignment m[x.i] = true passes x.i by reference, causing x to get marked as addrtaken.

Note that the logically equivalent program that replaces m[x.i] = true with tmp := x.i; m[tmp] = true does not create a stack object for x.
@mdempsky mdempsky added NeedsFix The path to resolution is known, but the work has not been done. FeatureRequest labels Jan 17, 2021
@mdempsky mdempsky added this to the Go1.17 milestone Jan 17, 2021
@mdempsky mdempsky self-assigned this Jan 17, 2021
@mdempsky
Copy link
Member Author

mdempsky commented Jan 17, 2021 •
edited

Slightly more realistic test case. x here should not require a stack object either:

package p

var m map[interface{}]bool

func f() {
	x := g()
	m[x] = true
	h()
}

func g() interface{}
func h()

(Edit: Actually, it's an autotmp copy of x that gets the stack object, not x itself. But my larger point is that f shouldn't need stack objects at all, since we statically know the lifetime of all variables there.)

@gopherbot
Copy link

Change https://golang.org/cl/284412 mentions this issue: [dev.regabi] cmd/compile: convert OPANIC argument to interface{} during typecheck

gopherbot pushed a commit that referenced this issue Jan 18, 2021
@mdempsky
[dev.regabi] cmd/compile: convert OPANIC argument to interface{} duri…
6113db0 
…ng typecheck

Currently, typecheck leaves arguments to OPANIC as their original
type. This CL changes it to insert implicit OCONVIFACE operations to
convert arguments to `interface{}` like how any other function call
would be handled.

No immediate benefits, other than getting to remove a tiny bit of
special-case logic in order.go's handling of OPANICs. Instead, the
generic code path for handling OCONVIFACE is used, if necessary.
Longer term, this should be marginally helpful for #43753, as it
reduces the number of cases where we need values to be addressable for
runtime calls.

However, this does require adding some hacks to appease existing
tests:

1. We need yet another kludge in inline budgeting, to ensure that
reflect.flag.mustBe stays inlinable for cmd/compile/internal/test's
TestIntendedInlining.

2. Since the OCONVIFACE expressions are now being introduced during
typecheck, they're now visible to escape analysis. So expressions like
"panic(1)" are now seen as "panic(interface{}(1))", and escape
analysis warns that the "interface{}(1)" escapes to the heap. These
have always escaped to heap, just now we're accurately reporting about
it.

(Also, unfortunately fmt.go hides implicit conversions by default in
diagnostics messages, so instead of reporting "interface{}(1) escapes
to heap", it actually reports "1 escapes to heap", which is
confusing. However, this confusing messaging also isn't new.)

Change-Id: Icedf60e1d2e464e219441b8d1233a313770272af
Reviewed-on: https://go-review.googlesource.com/c/go/+/284412
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
@mdempsky mdempsky modified the milestones: Go1.17, Backlog Apr 29, 2021
@mdempsky
Copy link
Member Author

So the issue here is that during walk we end up rewriting m[k] = v into roughly *mapassign(m, &k) = v. When we create the node for &k it marks k as addrtaken. In general, variables marked addrtaken require stack objects, because we don't track how long the pointer will stay alive.

But when we taking the address of a variable to pass it to a runtime function like mapassign, we often know the variable is going to stay live until the runtime function returns. In this case, we don't need the full generality of stack objects; instead, as long as we add VarLive ops after the call (or probably by using CallExpr.KeepAlive), then we can let liveness analysis and the stack maps take care of keeping the variable alive instead.

This probably requires splitting Addrtaken into two separate flags: one to represent that the variable "needs to be addressable" and a second to represent that "the variable needs a stack object to track its liveness". We would set both flags for variables that the user takes the address of. In at least some cases in later stages of the compiler, we may only need to set "needs to be addressable".

@gopherbot
Copy link

Change https://golang.org/cl/321711 mentions this issue: cmd/compile: add new flag to track whether variables need stack objects

@gopherbot
Copy link

Change https://golang.org/cl/321712 mentions this issue: cmd/compile: mark OVARLIVE variables not require stack objects

@gopherbot
Copy link

Change https://golang.org/cl/321714 mentions this issue: cmd/compile: do not mark stack object for map key passed to runtime function

@gopherbot
Copy link

Change https://golang.org/cl/321713 mentions this issue: cmd/compile: refactor walkIndexMap

gopherbot pushed a commit that referenced this issue Mar 24, 2022
@cuonglm
cmd/compile: refactor walkIndexMap
209521c 
So all runtime map functions will use the same code path, make it easier
for future CL to handle stack object for map key.

Passes toolstash -cmp.

For #43753

Change-Id: I374fa4e351c1eba079e2ccb637b1ef5adad1488f
Reviewed-on: https://go-review.googlesource.com/c/go/+/321713
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
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. FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Backlog
Development

No branches or pull requests
2 participants
@mdempsky @gopherbot