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: clumsy rematerialization in mapassign_fast* #19733

Open
philhofer opened this issue Mar 28, 2017 · 4 comments
Open

cmd/compile: clumsy rematerialization in mapassign_fast* #19733

philhofer opened this issue Mar 28, 2017 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Performance
Milestone
Unplanned

Comments

@philhofer
Copy link
Contributor

philhofer commented Mar 28, 2017 •
edited

On 73912a1

If you disassemble runtime.mapassign_fast32 on ARM, you'll find the following code generated for the bucket probing loop:

  0.34 │ d4:   cmp    ip, #8
       │     ↓ bcs    1a0
       │       ldrsb  fp, [r2]
       │       add    lr, r2, ip
       │       ldrb   r0, [lr] 
       │       and    r3, r5, #255
       │       cmp    r3, r0
       │     ↓ beq    13c
       │       cmp    r0, #0
 39.68 │     ↓ bne    104
       │       cmp    r7, #0
       │     ↓ beq    114              ; jumps just past this loop
       │104:   add    ip, ip, #1
  0.20 │       ldr    r0, [sp, #28]    ; <-- clobbered in loop
       │       mov    r3, #1           ; <-- clobbered in loop
       │     ↑ b      d4

(The ldr r0, [sp, #28] is reloading a spill of the variable bucket.)

The beq 114 branch jumps forwards to a block that uses r0 and r3. I'd expect the values in those registers to be rematerialized in that block, rather than in the body of a loop. (They don't appear to be live on any other edges out of this loop.)

Found while investigating #19495
@josharian
Copy link
Contributor

@cherrymui

@josharian josharian added this to the Go1.9Maybe milestone Mar 28, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe May 24, 2017
@odeke-em
Copy link
Member

@randall77 I noticed that about 24 hours ago your CL https://go-review.googlesource.com/c/go/+/79018 and commit 48e207d was submitted, touching mapassign_fast*. I am just totally guessing a correlation/possible update,
but could you or @philhofer please check if this still some work to be done here? Thank you!

@randall77
Copy link
Contributor

I don't think my recent CL would change this, all the modifications were outside the probe loop.
Looking at disasembly, I see big changes at tip from the assembly listed here. I think the bucket variable is no longer restored in the loop, but the key we're searching for is.
So there's still work to be done.

@odeke-em
Copy link
Member

Awesome, thank you @randall77 for the update!

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 28, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 18, 2018
@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. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @josharian @philhofer @odeke-em @randall77 @gopherbot