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

cmd/compile: clumsy rematerialization in mapassign_fast* #19733

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

Comments

Projects
None yet
5 participants
@philhofer
Contributor

philhofer commented Mar 28, 2017

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

This comment has been minimized.

Contributor

josharian commented Mar 28, 2017

@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

This comment has been minimized.

Member

odeke-em commented Nov 23, 2017

@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

This comment has been minimized.

Contributor

randall77 commented Nov 23, 2017

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

This comment has been minimized.

Member

odeke-em commented Nov 23, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment