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: investigate wasm sync inlining #30605

Open
bradfitz opened this Issue Mar 5, 2019 · 2 comments

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented Mar 5, 2019

https://go-review.googlesource.com/c/go/+/148958/ made sync.Mutex.Unlock inlinable.

It worked for Wasm, but then CL 164461 and CL 153797 landed and the resulting merge no longer passed the inlining tests.

As a quick fix, https://go-review.googlesource.com/c/go/+/165339 ignored wasm in the inlining tests.

Investigate & re-enable.

/cc @ALTree @neelance @aclements @CAFxX

@gopherbot

This comment has been minimized.

Copy link

commented Mar 5, 2019

Change https://golang.org/cl/165339 mentions this issue: test: skip mutex Unlock inlining tests on a few builders

gopherbot pushed a commit that referenced this issue Mar 5, 2019

test: skip mutex Unlock inlining tests on a few builders
Fix builder breakage from CL 148958.

This is an inlining test that should be skipped on -N -l.

The inlining also doesn't happen on arm and wasm, so skip the test
there too.

Fixes the noopt builder, the linux-arm builder, and the wasm builder.

Updates #30605

Change-Id: I06b90d595be7185df61db039dd225dc90d6f678f
Reviewed-on: https://go-review.googlesource.com/c/go/+/165339
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@neelance

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

The test now explains:

// FIXME: This test is disabled on architectures where atomic operations
// are function calls rather than intrinsics, since this prevents inlining
// of the sync fast paths. This test should be re-enabled once the problem
// is solved.

This applies to wasm since it has no atomic operations yet. The test needs to be reenabled after the threads proposal landed.

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