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: SSA compiler removes code in benchmarks #14813

Closed
ainar-g opened this issue Mar 14, 2016 · 5 comments

Comments

@ainar-g
Copy link
Contributor

commented Mar 14, 2016

Go version: go version devel +2dcbbbd Mon Mar 14 05:13:47 2016 +0000 linux/amd64

Go env:

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

This code is a simple benchmark of Hamming weight algorithms: http://play.golang.org/p/kOid3lX9Yz

In Go 1.6 running go test popcnt_test.go -bench . gives the result:

testing: warning: no tests to run
PASS
BenchmarkPopcnt-4   50000000            27.1 ns/op
BenchmarkPopcnt2-4  1000000000           2.90 ns/op
ok      command-line-arguments  4.567s

But on tip with SSA turned on it's

testing: warning: no tests to run
BenchmarkPopcnt-4   100000000           24.0 ns/op
BenchmarkPopcnt2-4  2000000000           0.41 ns/op
PASS
ok      command-line-arguments  3.286s

Running with -gcflags "-S" confirms that the tip version completely removes the call to popcnt2. If I add //go:noinline before popcnt2 definition, it works fine (although slower than 1.6 because it doesn't inline):

testing: warning: no tests to run
BenchmarkPopcnt-4   50000000            22.9 ns/op
BenchmarkPopcnt2-4  300000000            4.42 ns/op
PASS
ok      command-line-arguments  2.946s

Same goes for running with -gcflags "-ssa=0", the call isn't removed.

@ainar-g ainar-g changed the title SSA compiler removes code in benchmarks cmd/compile: SSA compiler removes code in benchmarks Mar 14, 2016

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

You are not using the value from popcnt2, and because the function was inlined, the compiler can see the value is unused, so it removed the call entirely.

Here is something I wrote about writing a reliable benchmark, http://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go, hopefully it will be of use.

I am going to close this as this is not a bug. For general Go questions see https://golang.org/wiki/Questions. Thanks!

@davecheney davecheney closed this Mar 14, 2016

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

Huh, I've seen the _ = f(x) and even simple f(x) idiom in some Go benchmarks I've encountered here and there, and it worked. But apparently with SSA they're going to become useless benchmarks.

Using the top-level result variable solves the problem. I guess it's kind of a documentation issue. Either way, thank you as well.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

@bradfitz I think we should reopen this issue, or replace it with another as a placeholder to double check that all the benchmarks in the stdlib are armoured against the improving abilities of the SSA backend.

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

I've found a few cases of _ = f(x) in the stdlib using [the search](https://golang.org/search?q=%28?s%29for i := 0%3B i < b.N%3B i%2B%2B+{.\s%2B_ =), but all of them seem to be unaffected by the SSA so far.

Could the problem be solved with something similar to //go:noinline magic comment (//go:noremove)? Code emitted by go test for benchmarks could be marked that way to make sure the compiler won't remove the code inside the loop.

@minux

This comment has been minimized.

Copy link
Member

commented Mar 14, 2016

No, no more magical //go:XXX comments please.
Just add a global variable to sink the values.

It's beginning to sound like gcc's attribute,
and I really don't want to read Go programs like
this:

//go:do_not_optimize_this
for i := 0; i < 10; i++ {}
//go:unroll 10
for i := 0; i < 100; i++ {
f(i) //go:must_inline
}

@golang golang locked and limited conversation to collaborators Mar 14, 2017

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