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: unnecessary slice literal zeroing #45573

Open
icholy opened this issue Apr 14, 2021 · 4 comments
Open

cmd/compile: unnecessary slice literal zeroing #45573

icholy opened this issue Apr 14, 2021 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@icholy
Copy link

icholy commented Apr 14, 2021

Keyless slice literals zero the backing array and then immediately overwrite it. https://godbolt.org/z/5bx9TPo7n

What operating system and processor architecture are you using?

linux amd64

What did you do?

package main

func main() {
    a := []int{1, 2, 3, 4}
    println(a)
}

What did you expect to see?

movq    $1, ""..autotmp_2+24(SP)
movq    $2, ""..autotmp_2+32(SP)
movq    $3, ""..autotmp_2+40(SP)
movq    $4, ""..autotmp_2+48(SP)

What did you see instead?

leaq    ""..autotmp_2+24(SP), AX
xorps   X15, X15
movups  X15, (AX)
leaq    ""..autotmp_2+40(SP), CX
xorps   X15, X15
movups  X15, (CX)
movq    $1, ""..autotmp_2+24(SP)
movq    $2, ""..autotmp_2+32(SP)
movq    $3, ""..autotmp_2+40(SP)
movq    $4, ""..autotmp_2+48(SP)
@randall77
Copy link
Contributor

Yes, the deadstore pass needs to be upgraded to handle cases like this. It's been on the back burner for a while.

I think there are 2 things that are needed to make this work:

  1. Instead of just recording the size of shadowed memory at each pointer, record an offset+size. Then we can fold OffPtr arguments into the analysis easily.
  2. Handle LocalAddr ops somehow - these are currently treated as reads and mess up the analysis. The code conservatively assumes taking the address makes the zeroed temp visible, when in fact that address is only used for storing.

I think 1 is easy. 2 might be a bit harder.

@randall77 randall77 added this to the Unplanned milestone Apr 14, 2021
@randall77 randall77 added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 14, 2021
@icholy
Copy link
Author

icholy commented Apr 18, 2021 •
edited

My initial though was to handle this at the ir/walk stages. Once in SSA form, the deadstore pass would need to re-discover the fact that the zeroed memory is shadowed.

@networkimprov
Copy link

cc @josharian

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@gopherbot
Copy link

Change https://go.dev/cl/538595 mentions this issue: cmd/compile: handle constant pointer offsets in dead store elimination

gopherbot pushed a commit that referenced this issue Oct 31, 2023
@randall77
cmd/compile: handle constant pointer offsets in dead store elimination
43b57b8 
Update #63657
Update #45573

Change-Id: I163c6038c13d974dc0ca9f02144472bc05331826
Reviewed-on: https://go-review.googlesource.com/c/go/+/538595
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
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. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@networkimprov @icholy @randall77 @gopherbot