This patch changes the inliner to use callsite scores when deciding to
inline as opposed to looking only at callee cost/hairyness.
For this to work, we have to relax the inline budget cutoff as part of
CanInline to allow for the possibility that a given function might
start off with a cost of N where N > 80, but then be called from a
callsites whose score is less than 80. Once a given function F in
package P has been approved by CanInline (based on the relaxed budget)
it will then be emitted as part of the export data, meaning that other
packages importing P will need to also need to compute callsite scores
appropriately.
For a function F that calls function G, if G is marked as potentially
inlinable then the hairyness computation for F will use G's cost for
the call to G as opposed to the default call cost; for this to work
with the new scheme (given relaxed cost change described above) we
use G's cost only if it falls below inlineExtraCallCost, otherwise
just use inlineExtraCallCost.
Included in this patch are a bunch of skips and workarounds to
selected 'errorcheck' tests in the <GOROOT>/test directory to deal
with the additional "can inline" messages emitted when the new inliner
is turned on.
Change-Id: I9be5f8cd0cd8676beb4296faf80d2f6be7246335
Reviewed-on: https://go-review.googlesource.com/c/go/+/519197
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
For constants literal, iimport/iexport read/write them as basic literal
nodes. So they are printed in diagnostic message as Go syntax. So "foo"
will be reported as string("foo").
Unified IR read/write the raw expression as string value, and when
printed in diagnostic, the string value is written out exactly as-is, so
"foo" will be written as "foo".
Thus, this CL relax the test in issue7921.go to match the string value only.
Updates #53058
Change-Id: I6fcf4fdcfc4b3be91cb53b081c48bd57186d8f35
Reviewed-on: https://go-review.googlesource.com/c/go/+/410795
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Currently, typecheck leaves arguments to OPANIC as their original
type. This CL changes it to insert implicit OCONVIFACE operations to
convert arguments to `interface{}` like how any other function call
would be handled.
No immediate benefits, other than getting to remove a tiny bit of
special-case logic in order.go's handling of OPANICs. Instead, the
generic code path for handling OCONVIFACE is used, if necessary.
Longer term, this should be marginally helpful for #43753, as it
reduces the number of cases where we need values to be addressable for
runtime calls.
However, this does require adding some hacks to appease existing
tests:
1. We need yet another kludge in inline budgeting, to ensure that
reflect.flag.mustBe stays inlinable for cmd/compile/internal/test's
TestIntendedInlining.
2. Since the OCONVIFACE expressions are now being introduced during
typecheck, they're now visible to escape analysis. So expressions like
"panic(1)" are now seen as "panic(interface{}(1))", and escape
analysis warns that the "interface{}(1)" escapes to the heap. These
have always escaped to heap, just now we're accurately reporting about
it.
(Also, unfortunately fmt.go hides implicit conversions by default in
diagnostics messages, so instead of reporting "interface{}(1) escapes
to heap", it actually reports "1 escapes to heap", which is
confusing. However, this confusing messaging also isn't new.)
Change-Id: Icedf60e1d2e464e219441b8d1233a313770272af
Reviewed-on: https://go-review.googlesource.com/c/go/+/284412
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Historically, inline function bodies were exported as plain Go source
code, and symbol mangling was a convenient hack because it allowed
variables to be re-imported with largely the same names as they were
originally exported as.
However, nowadays we use a binary format that's more easily extended,
so we can simply serialize all of a function's declared objects up
front, and then refer to them by index later on. This also allows us
to easily report unmangled names all the time (e.g., error message
from issue7921.go).
Fixes#43633.
Change-Id: I46c88f5a47cb921f70ab140976ba9ddce38df216
Reviewed-on: https://go-review.googlesource.com/c/go/+/283193
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Trust: Dan Scales <danscales@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
This CL changes "T literal.M" error message to "T{...}.M". It's clearer
expression and focusing user on actual issue.
Updates #38745
Change-Id: I84b455a86742f37e0bde5bf390aa02984eecc3c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/253677
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This information is redundant with the position information already
provided. Also, no other -m diagnostics print out function name.
While here, report parameter leak diagnostics against the parameter
declaration position rather than the function, and use Warnl for
"moved to heap" messages.
Test cases updated programmatically by removing the first word from
every "no match for" error emitted by run.go:
go run run.go |& \
sed -E -n 's/^(.*):(.*): no match for `([^ ]* (.*))` in:$/\1!\2!\3!\4/p' | \
while IFS='!' read -r fn line before after; do
before=$(echo "$before" | sed 's/[.[\*^$()+?{|]/\\&/g')
after=$(echo "$after" | sed -E 's/(\&|\\)/\\&/g')
fn=$(find . -name "${fn}" | head -1)
sed -i -E -e "${line}s/\"${before}\"/\"${after}\"/" "${fn}"
done
Passes toolstash-check.
Change-Id: I6e02486b1409e4a8dbb2b9b816d22095835426b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/195040
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
For most nodes (e.g., OPTRLIT, OMAKESLICE, OCONVIFACE), escape
analysis prints "escapes to heap" or "does not escape" to indicate
whether that node's allocation can be heap or stack allocated.
These messages are also emitted for OADDR, even though OADDR does not
actually allocate anything itself. Moreover, it's redundant because
escape analysis already prints "moved to heap" diagnostics when an
OADDR node like "&x" causes x to require heap allocation.
Because OADDR nodes don't allocate memory, my escape analysis rewrite
doesn't naturally emit the "escapes to heap" / "does not escape"
diagnostics for them. It's also non-trivial to replicate the exact
semantics esc.go uses for OADDR.
Since there are so many of these messages, I'm disabling them in this
CL by themselves. I modified esc.go to suppress the Warnl calls
without any other behavior changes, and then used a shell script to
automatically remove any ERROR messages mentioned by run.go in
"missing error" or "no match for" lines.
Fixes#16300.
Updates #23109.
Change-Id: I3993e2743c3ff83ccd0893f4e73b366ff8871a57
Reviewed-on: https://go-review.googlesource.com/c/go/+/170319
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
A Go user made a well-documented request for a slightly
lower threshold. I tested against a selection of other
people's benchmarks, and saw a tiny benefit (possibly noise)
at equally tiny cost, and no unpleasant surprises observed
in benchmarking.
I.e., might help, doesn't hurt, low risk, request was
delivered on a silver platter.
It did, however, change the behavior of one test because
now bytes.Buffer.Grow is eligible for inlining.
Updates #19348.
Change-Id: I85e3088a4911290872b8c6bda9601b5354c48695
Reviewed-on: https://go-review.googlesource.com/c/151977
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This is a simple tweak to allow a bit more mid-stack inlining.
In cases like this:
func f() {
g()
}
We'd really like to inline f into its callers. It can't hurt.
We implement this optimization by making calls a bit cheaper, enough
to afford a single call in the function body, but not 2.
The remaining budget allows for some argument modification, or perhaps
a wrapping conditional:
func f(x int) {
g(x, 0)
}
func f(x int) {
if x > 0 {
g()
}
}
Update #19348
Change-Id: Ifb1ea0dd1db216c3fd5c453c31c3355561fe406f
Reviewed-on: https://go-review.googlesource.com/c/147361
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: David Chase <drchase@google.com>
Adds a new build tag "gcflags_noopt" that can be used in test/*.go
tests.
Fixes#27833
Change-Id: I4ea0ccd9e9e58c4639de18645fec81eb24a3a929
Reviewed-on: https://go-review.googlesource.com/136898
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Added some more cases that should be guarded against regression.
Change-Id: I9f1dda2fd0be9b6e167ef1cc018fc8cce55c066c
Reviewed-on: https://go-review.googlesource.com/134017
Run-TryBot: Iskander Sharipov <iskander.sharipov@intel.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Rationale: small buffer optimization does not work and it has
made things slower since 2014. Until we can make it work,
we should prefer simpler code that also turns out to be more
efficient.
With this change, it's possible to use
NewBuffer(make([]byte, 0, bootstrapSize)) to get the desired
stack-allocated initial buffer since escape analysis can
prove the created slice to be non-escaping.
New implementation key points:
- Zero value bytes.Buffer performs better than before
- You can have a truly stack-allocated buffer, and it's not even limited to 64 bytes
- The unsafe.Sizeof(bytes.Buffer{}) is reduced significantly
- Empty writes don't cause allocations
Buffer benchmarks from bytes package:
name old time/op new time/op delta
ReadString-8 9.20µs ± 1% 9.22µs ± 1% ~ (p=0.148 n=10+10)
WriteByte-8 28.1µs ± 0% 26.2µs ± 0% -6.78% (p=0.000 n=10+10)
WriteRune-8 64.9µs ± 0% 65.0µs ± 0% +0.16% (p=0.000 n=10+10)
BufferNotEmptyWriteRead-8 469µs ± 0% 461µs ± 0% -1.76% (p=0.000 n=9+10)
BufferFullSmallReads-8 108µs ± 0% 108µs ± 0% -0.21% (p=0.000 n=10+10)
name old speed new speed delta
ReadString-8 3.56GB/s ± 1% 3.55GB/s ± 1% ~ (p=0.165 n=10+10)
WriteByte-8 146MB/s ± 0% 156MB/s ± 0% +7.26% (p=0.000 n=9+10)
WriteRune-8 189MB/s ± 0% 189MB/s ± 0% -0.16% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
ReadString-8 32.8kB ± 0% 32.8kB ± 0% ~ (all equal)
WriteByte-8 0.00B 0.00B ~ (all equal)
WriteRune-8 0.00B 0.00B ~ (all equal)
BufferNotEmptyWriteRead-8 4.72kB ± 0% 4.67kB ± 0% -1.02% (p=0.000 n=10+10)
BufferFullSmallReads-8 3.44kB ± 0% 3.33kB ± 0% -3.26% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
ReadString-8 1.00 ± 0% 1.00 ± 0% ~ (all equal)
WriteByte-8 0.00 0.00 ~ (all equal)
WriteRune-8 0.00 0.00 ~ (all equal)
BufferNotEmptyWriteRead-8 3.00 ± 0% 3.00 ± 0% ~ (all equal)
BufferFullSmallReads-8 3.00 ± 0% 2.00 ± 0% -33.33% (p=0.000 n=10+10)
The most notable thing in go1 benchmarks is reduced allocs in HTTPClientServer (-1 alloc):
HTTPClientServer-8 64.0 ± 0% 63.0 ± 0% -1.56% (p=0.000 n=10+10)
For more explanations and benchmarks see the referenced issue.
Updates #7921
Change-Id: Ica0bf85e1b70fb4f5dc4f6a61045e2cf4ef72aa3
Reviewed-on: https://go-review.googlesource.com/133715
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
