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.

Sign up for GitHub

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: functions with switches cannot be inlined #13071

Closed
dsnet opened this issue Oct 27, 2015 · 8 comments
Closed

cmd/compile: functions with switches cannot be inlined #13071

dsnet opened this issue Oct 27, 2015 · 8 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented Oct 27, 2015

Using go1.5

Consider the following package:

package main

type dictDecoder struct {
    hist []byte
    wrPos int
}

func (dd *dictDecoder) LastBytesA() (p1, p2 byte) {
    if dd.wrPos > 1 {
        return dd.hist[dd.wrPos-1], dd.hist[dd.wrPos-2]
    } else if dd.wrPos > 0 {
        return dd.hist[dd.wrPos-1], dd.hist[len(dd.hist)-1]
    } else {
        return dd.hist[len(dd.hist)-1], dd.hist[len(dd.hist)-2]
    }
}

func (dd *dictDecoder) LastBytesB() (p1, p2 byte) {
    switch {
    case dd.wrPos > 1:
        return dd.hist[dd.wrPos-1], dd.hist[dd.wrPos-2]
    case dd.wrPos > 0:
        return dd.hist[dd.wrPos-1], dd.hist[len(dd.hist)-1]
    default:
        return dd.hist[len(dd.hist)-1], dd.hist[len(dd.hist)-2]
    }
}

Using go build -gcflags=-S dict.go, I see that LastBytesA and LastBytesB produce the exact same assembly output.

However, go build -gcflags=-m dict.go reports that only LastByteA can be inlined and not LastBytesB:

# command-line-arguments
./main4.go:8: can inline (*dictDecoder).LastBytesA
./main4.go:8: (*dictDecoder).LastBytesA dd does not escape
./main4.go:18: (*dictDecoder).LastBytesB dd does not escape

I expect both LastBytesA and LastBytesB to be inline-able if they produce the same assembly output. This would be nice since (IMHO), switch statements are cleaner for some if-elseif-elseif-elseif-else blocks.
@bradfitz bradfitz added this to the Unplanned milestone Oct 27, 2015
@randall77
Copy link
Contributor

This would be nice as part of a general revisit of inlining decisions. I don't think this is on anyone's radar at the moment, though.

See #12312 We'll need noinline directives first so we can stop using switch{} as a noinline hint (should happen soon).

pwiecz referenced this issue Oct 29, 2015
@bradfitz
math: replace assembly implementations of Abs with pure Go version
6f8a665 
The compiler can do a fine job, and can also inline it.

From Jeremy Jackins's observation and rsc's recommendation in thread:

"Pure Go math.Abs outperforms assembly version"
https://groups.google.com/forum/#!topic/golang-dev/nP5mWvwAXZo

Updates #13095

Change-Id: I3066f8eaa327bb403173b29791cc8661d7c0532c
Reviewed-on: https://go-review.googlesource.com/16444
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

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

bradfitz added a commit that referenced this issue Oct 29, 2015
@bradfitz
math: fix typo and braino in my earlier commit
a59a275 
The bug number was a typo, and I forgot to switch the implementation
back to if statements after the change from Float64bits in the first
patchset back to branching.

if statements can currently be inlined, but switch cannot (#13071)

Change-Id: I81d0cf64bda69186c3d747a07047f6a694f8fa70
Reviewed-on: https://go-review.googlesource.com/16446
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dsnet dsnet mentioned this issue Nov 22, 2015
Merged
@gopherbot
Copy link

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

@randall77
Copy link
Contributor

A bunch of tests until recently used switch{} to disable inlining. We just added a go:noinline directive to replace these uses. We'll need to make sure we replace all of the switch{} uses and then we can fix this bug.

@dsnet
Copy link
Member Author

dsnet commented Nov 22, 2015

That sounds wonderful. Would that also fix #7655? And would this make it into go1.6?

Regardless, I'd be happy to abandon that CL of mine.

@dsnet
Copy link
Member Author

dsnet commented Nov 22, 2015

@randall77 You mentioned that it might be a good time to revisit inline decisions. Is that taking placing somewhere? I would be interested in being involved in that discussion. I've been working on trying to make compression libraries more performant, and many of the changes I make are related to coercing the compiler to inline certain functions.

@klauspost would probably be interested in these discussions as well.

@randall77
Copy link
Contributor

I know of no one working on inline decisions right now.
This bug and #7655 should be pretty easy to fix.
Any fix would not make 1.6, as we are already in the freeze.

rsc pushed a commit that referenced this issue Nov 26, 2015
@dsnet @rsc
compress/flate: tweak offsetCode so that it can be inlined
b717090 
Functions with switches (#13071) cannot be inlined.
Functions with consts (#7655) cannot be inlined.

benchmark                              old MB/s     new MB/s     speedup
BenchmarkEncodeDigitsSpeed1e4-4        10.25        10.20        1.00x
BenchmarkEncodeDigitsSpeed1e5-4        26.44        27.22        1.03x
BenchmarkEncodeDigitsSpeed1e6-4        32.28        33.51        1.04x
BenchmarkEncodeDigitsDefault1e4-4      8.61         8.74         1.02x
BenchmarkEncodeDigitsDefault1e5-4      7.03         6.98         0.99x
BenchmarkEncodeDigitsDefault1e6-4      6.47         6.46         1.00x
BenchmarkEncodeDigitsCompress1e4-4     8.62         8.73         1.01x
BenchmarkEncodeDigitsCompress1e5-4     7.01         6.98         1.00x
BenchmarkEncodeDigitsCompress1e6-4     6.43         6.53         1.02x
BenchmarkEncodeTwainSpeed1e4-4         9.67         10.16        1.05x
BenchmarkEncodeTwainSpeed1e5-4         26.46        26.94        1.02x
BenchmarkEncodeTwainSpeed1e6-4         33.19        34.02        1.03x
BenchmarkEncodeTwainDefault1e4-4       8.12         8.37         1.03x
BenchmarkEncodeTwainDefault1e5-4       8.22         8.21         1.00x
BenchmarkEncodeTwainDefault1e6-4       8.10         8.13         1.00x
BenchmarkEncodeTwainCompress1e4-4      8.24         8.39         1.02x
BenchmarkEncodeTwainCompress1e5-4      6.51         6.58         1.01x
BenchmarkEncodeTwainCompress1e6-4      6.16         6.13         1.00x

Change-Id: Ibafa5e3e2de0529853b5b3180e6fd6cb7090b76f
Reviewed-on: https://go-review.googlesource.com/17171
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Mar 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @dsnet @randall77 @gopherbot