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

cmd/compile: reordering regular loads over atomic add #20335

Closed
aclements opened this issue May 11, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@aclements
Copy link
Member

commented May 11, 2017

What version of Go are you using (go version)?

Tested on both go version go1.8.1 linux/amd64 and go version devel +8c49c06b48 Mon May 8 15:16:21 2017 +0000 linux/amd64.

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

runtime.gchelper ends with the following few lines:

	nproc := work.nproc // work.nproc can change right after we increment work.ndone
	if atomic.Xadd(&work.ndone, +1) == nproc-1 {
		notewakeup(&work.alldone)
	}

What did you expect to see?

I expected the atomic.Xadd to act as a memory fence and force the regular load of work.nproc to happen before the atomic.

What did you see instead?

The compiler reorders the regular load to occur after the atomic.Xadd. I believe this is leading to #19305.

/cc @dr2chase @cherrymui

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

Minimal repro:

package main

import "sync/atomic"

func f(p, q *int32) bool {
	x := *q
	return atomic.AddInt32(p, 1) == x
}

The *q occurs after the atomic. Something is wrong with the schedule pass

    v9 = XADDLlock <uint32,mem> v12 v7 v1
    v5 = Select0 <uint32> v9
    v14 = Select1 <mem> v9
    v11 = MOVLload <int32> v8 v1

There's more than one memory variable live before v11 (v1 and v14).

@randall77 randall77 self-assigned this May 11, 2017

@gopherbot

This comment has been minimized.

Copy link

commented May 11, 2017

CL https://golang.org/cl/43294 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented May 11, 2017

CL https://golang.org/cl/43311 mentions this issue.

@gopherbot gopherbot closed this in 978af9c May 11, 2017

@randall77 randall77 reopened this May 11, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

Reopen for 1.8.2.

@gopherbot

This comment has been minimized.

Copy link

commented May 12, 2017

CL https://golang.org/cl/43411 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2017

CL https://golang.org/cl/43495 mentions this issue.

gopherbot pushed a commit that referenced this issue May 15, 2017

cmd/compile: better check for single live memory
Enhance the one-live-memory-at-a-time check to run during many
more phases of the SSA backend. Also make it work in an interblock
fashion.

Change types.IsMemory to return true for tuples containing a memory type.

Fix trim pass to build the merged phi correctly. Doesn't affect
code but allows the check to pass after trim runs.

Switch the AddTuple* ops to take the memory-containing tuple argument second.

Update #20335

Change-Id: I5b03ef3606b75a9e4f765276bb8b183cdc172b43
Reviewed-on: https://go-review.googlesource.com/43495
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

@bradfitz bradfitz modified the milestones: Go1.8.2, Go1.8.3 May 18, 2017

gopherbot pushed a commit that referenced this issue May 23, 2017

[release-branch.go1.8] cmd/compile: fix store chain in schedule pass
Cherry-pick of CL 43294.

Tuple ops are weird. They are essentially a pair of ops,
one which consumes a mem and one which generates a mem (the Select1).
The schedule pass didn't handle these quite right.

Fix the scheduler to include both parts of the paired op in
the store chain. That makes sure that loads are correctly ordered
with respect to the first of the pair.

Add a check for the ssacheck builder, that there is only one
live store at a time. I thought we already had such a check, but
apparently not...

Fixes #20335

Change-Id: I59eb3446a329100af38d22820b1ca2190ca46a78
Reviewed-on: https://go-review.googlesource.com/43411
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

@broady broady closed this May 23, 2017

@golang golang locked and limited conversation to collaborators May 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.