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 change does a bulk rename of several identifiers in the compiler.
See #27167 and https://docs.google.com/document/d/19_ExiylD9MRfeAjKIfEsMU1_RGhuxB9sA0b5Zv7byVI/
for context and for discussion of these particular renames.
Commands run to generate this change:
gorename -from '"cmd/compile/internal/gc".OPROC' -to OGO
gorename -from '"cmd/compile/internal/gc".OCOM' -to OBITNOT
gorename -from '"cmd/compile/internal/gc".OMINUS' -to ONEG
gorename -from '"cmd/compile/internal/gc".OIND' -to ODEREF
gorename -from '"cmd/compile/internal/gc".OARRAYBYTESTR' -to OBYTES2STR
gorename -from '"cmd/compile/internal/gc".OARRAYBYTESTRTMP' -to OBYTES2STRTMP
gorename -from '"cmd/compile/internal/gc".OARRAYRUNESTR' -to ORUNES2STR
gorename -from '"cmd/compile/internal/gc".OSTRARRAYBYTE' -to OSTR2BYTES
gorename -from '"cmd/compile/internal/gc".OSTRARRAYBYTETMP' -to OSTR2BYTESTMP
gorename -from '"cmd/compile/internal/gc".OSTRARRAYRUNE' -to OSTR2RUNES
gorename -from '"cmd/compile/internal/gc".Etop' -to ctxStmt
gorename -from '"cmd/compile/internal/gc".Erv' -to ctxExpr
gorename -from '"cmd/compile/internal/gc".Ecall' -to ctxCallee
gorename -from '"cmd/compile/internal/gc".Efnstruct' -to ctxMultiOK
gorename -from '"cmd/compile/internal/gc".Easgn' -to ctxAssign
gorename -from '"cmd/compile/internal/gc".Ecomplit' -to ctxCompLit
Not altered: parameters and local variables (mostly in typecheck.go) named top,
which should probably now be called ctx (and which should probably have a named type).
Also not altered: Field called Top in gc.Func.
gorename -from '"cmd/compile/internal/gc".Node.Isddd' -to IsDDD
gorename -from '"cmd/compile/internal/gc".Node.SetIsddd' -to SetIsDDD
gorename -from '"cmd/compile/internal/gc".nodeIsddd' -to nodeIsDDD
gorename -from '"cmd/compile/internal/types".Field.Isddd' -to IsDDD
gorename -from '"cmd/compile/internal/types".Field.SetIsddd' -to SetIsDDD
gorename -from '"cmd/compile/internal/types".fieldIsddd' -to fieldIsDDD
Not altered: function gc.hasddd, params and local variables called isddd
Also not altered: fmt.go prints nodes using "isddd(%v)".
cd cmd/compile/internal/gc; go generate
I then manually found impacted comments using exact string match
and fixed them up by hand. The comment changes were trivial.
Passes toolstash-check.
Fixes#27167. If this experiment is deemed a success,
we will open a new tracking issue for renames to do
at the end of the 1.13 cycles.
Change-Id: I2dc541533d2ab0d06cb3d31d65df205ecfb151e8
Reviewed-on: https://go-review.googlesource.com/c/150140
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
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>
Avoids allocating an ONAME for OLABEL, OGOTO, and named OBREAK and
OCONTINUE nodes.
Passes toolstash-check.
Change-Id: I359142cd48e8987b5bf29ac100752f8c497261c1
Reviewed-on: https://go-review.googlesource.com/c/145200
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Nowadays there are better ways to safely run untrusted Go programs, like
NaCl and gVisor.
Change-Id: I20c45f13a50dbcf35c343438b720eb93e7b4e13a
Reviewed-on: https://go-review.googlesource.com/c/142717
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Prior to stack tracing, inlining could cause
dead pointers to be kept alive in some loops.
See #18336 and CL 31674.
The adjustment removed by this change preserved the inlining status quo
in the face of Node structure changes, to avoid creating new problems.
Now that stack tracing provides precision, these hacks can be removed.
Of course, our inlining code model is already hacky (#17566),
but at least now there will be fewer epicyclical hacks.
Newly inline-able functions in std cmd as a result of this change:
hash/adler32/adler32.go:65:6: can inline (*digest).UnmarshalBinary
hash/fnv/fnv.go:281:6: can inline (*sum32).UnmarshalBinary
hash/fnv/fnv.go:292:6: can inline (*sum32a).UnmarshalBinary
reflect/value.go:1298:6: can inline Value.OverflowComplex
compress/bzip2/bit_reader.go:25:6: can inline newBitReader
encoding/xml/xml.go:365:6: can inline (*Decoder).switchToReader
vendor/golang_org/x/crypto/cryptobyte/builder.go:77:6: can inline (*Builder).AddUint16
crypto/x509/x509.go:1851:58: can inline buildExtensions.func2.1.1
crypto/x509/x509.go:1871:58: can inline buildExtensions.func2.3.1
crypto/x509/x509.go:1883:58: can inline buildExtensions.func2.4.1
cmd/vet/internal/cfg/builder.go:463:6: can inline (*builder).labeledBlock
crypto/tls/handshake_messages.go:1450:6: can inline (*newSessionTicketMsg).marshal
crypto/tls/handshake_server.go:769:6: can inline (*serverHandshakeState).clientHelloInfo
crypto/tls/handshake_messages.go:1171:6: can inline (*nextProtoMsg).unmarshal
cmd/link/internal/amd64/obj.go:40:6: can inline Init
cmd/link/internal/ppc64/obj.go:40:6: can inline Init
net/http/httputil/persist.go:54:6: can inline NewServerConn
net/http/fcgi/child.go:83:6: can inline newResponse
cmd/compile/internal/ssa/poset.go:245:6: can inline (*poset).newnode
Change-Id: I19e8e383a6273849673d35189a9358870665f82f
Reviewed-on: https://go-review.googlesource.com/c/141117
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The new indexed package export format appears stable, and no reports
of needing to revert back to binary package export.
This CL disables the binary package export format by mechanically
replacing 'flagiexport' with 'true', and then superficial code
cleanups to keep the resulting code idiomatic. The resulting dead code
is removed in a followup CL.
Change-Id: Ic30d85f78778a31d279a56b9ab14e80836d50135
Reviewed-on: https://go-review.googlesource.com/c/139337
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
This CL adds the source code of all inlined functions
into the function specified in $GOSSAFUNC.
The code is appended to the sources column of ssa.html.
ssaDumpInlined is populated with references to inlined functions.
Then it is used for dumping the sources in buildssa.
The source columns contains code in following order:
target function, inlined functions sorted by filename, lineno.
Fixes#25904
Change-Id: I4f6d4834376f1efdfda1f968a5335c0543ed36bc
Reviewed-on: https://go-review.googlesource.com/126606
Run-TryBot: Yury Smolsky <yury@smolsky.by>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
If we're compiling a large function, be more picky about how big
the function we're inlining is. If the function is >5000 nodes,
we lower the inlining threshold from a cost of 80 to 20.
Turns out reflect.Value's cost is exactly 80. That's the function
at issue in #26546.
20 was chosen as a proxy for "inlined body is smaller than the call would be".
Simple functions still get inlined, like this one at cost 7:
func ifaceIndir(t *rtype) bool {
return t.kind&kindDirectIface == 0
}
5000 nodes was chosen as the big function size. Here are all the
5000+ node (~~1000+ lines) functions in the stdlib:
5187 cmd/internal/obj/arm (*ctxt5).asmout
6879 cmd/internal/obj/s390x (*ctxtz).asmout
6567 cmd/internal/obj/ppc64 (*ctxt9).asmout
9643 cmd/internal/obj/arm64 (*ctxt7).asmout
5042 cmd/internal/obj/x86 (*AsmBuf).doasm
8768 cmd/compile/internal/ssa rewriteBlockAMD64
8878 cmd/compile/internal/ssa rewriteBlockARM
8344 cmd/compile/internal/ssa rewriteValueARM64_OpARM64OR_20
7916 cmd/compile/internal/ssa rewriteValueARM64_OpARM64OR_30
5427 cmd/compile/internal/ssa rewriteBlockARM64
5126 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_50
6152 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_60
6412 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_70
6486 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_80
6534 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_90
6534 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_100
6534 cmd/compile/internal/ssa rewriteValuePPC64_OpPPC64OR_110
6675 cmd/compile/internal/gc typecheck1
5433 cmd/compile/internal/gc walkexpr
14070 cmd/vendor/golang.org/x/arch/arm64/arm64asm decodeArg
There are a lot more smaller (~1000 node) functions in the stdlib.
The function in #26546 has 12477 nodes.
At some point it might be nice to have a better heuristic for "inlined
body is smaller than the call", a non-cliff way to scale down the cost
as the function gets bigger, doing cheaper inlined calls first, etc.
All that can wait for another release. I'd like to do this CL for
1.11.
Fixes#26546
Update #17566
Change-Id: Idda13020e46ec2b28d79a17217f44b189f8139ac
Reviewed-on: https://go-review.googlesource.com/125516
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Adds the appropriate check to inl.go.
Includes tests of both -race+go:norace and plain go:norace.
Fixes#24651.
Change-Id: Id806342430c20baf4679a985d12eea3b677092e0
Reviewed-on: https://go-review.googlesource.com/119195
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This line of the inlining tuning experiment
https://go-review.googlesource.com/c/go/+/109918/1/src/cmd/compile/internal/gc/inl.go#347
was incorrectly rewritten in a later patch to use the call
cost, not the panic cost, and thus the inlining of panic
didn't occur when it should. I discovered this when I
realized that tests should have failed, but didn't.
Fix is to make the correct change, and also to modify the
tests that this causes to fail. One test now asserts the
new normal, the other calls "ppanic" instead which is
designed to behave like panic but not be inlined.
Change-Id: I423bb7f08bd66a70d999826dd9b87027abf34cdf
Reviewed-on: https://go-review.googlesource.com/116656
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Fix a problem in DWARF inline debug generation relating to handling of
statically unreachable local variables. For a function such as:
const always = true
func HasDeadLocal() int {
if always {
return 9
}
x := new(Something)
...
return x.y
}
the variable "x" is placed onto the Dcl list for the function during
parsing, but the actual declaration node is deleted later on when
gc.Main invokes "deadcode". Later in the compile the DWARF code emits
an abstract function with "x" (since "x" was on the Dcl list at the
point of the inline), but the export data emitted does not contain
"x". This then creates clashing/inconsistant DWARF abstract function
DIEs later on if HasDeadLocal is inlined in somewhere else.
As a fix, the inliner now pruned away variables such as "x" when
creating a copy of the Dcl list as part of the inlining; this means
that both the export data generator and the DWARF emitter wind up
seeing a consistent picture.
Fixes#25459
Change-Id: I753dc4e9f9ec694340adba5f43c907ba8cc9badc
Reviewed-on: https://go-review.googlesource.com/114090
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Inlining was refactored to perform tuning experiments,
with the "knobs" now set to also inline functions/methods
that include panic(), and -l=4 (inline calls) now expressed
as a change to costs, rather than scattered if-thens.
The -l=4 inline-calls penalty is chosen to be the best
found during experiments; it makes some programs much
larger and slower (notably, the compiler itself) and is
believed to be risky for machine-generated code in general,
which is why it is not the default. It is also not
well-tested with the debugger and DWARF output.
This change includes an explicit go:noinline applied to the
method that is the largest cause of compiler binary growth
and slowdown for midstack inlining; there are others,
ideally whatever heuristic eventually appears will make
this unnecessary.
Change-Id: Idf7056ed2f961472cf49d2fd154ee98bef9421e2
Reviewed-on: https://go-review.googlesource.com/109918
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
For struct fields and methods, Field.Nname was only used to store
position information, which means we're allocating an entire ONAME
Node+Name+Param structure just for one field. We can optimize away
these ONAME allocations by instead adding a Field.Pos field.
Unfortunately, we can't get rid of Field.Nname, because it's needed
for function parameters, so Field grows a little bit and now has more
redundant information in those cases. However, that was already the
case (e.g., Field.Sym and Field.Nname.Sym), and it's still a net win
for allocations as demonstrated by the benchmarks below.
Additionally, by moving the ONAME allocation for function parameters
to funcargs, we can avoid allocating them for function parameters that
aren't used in corresponding function bodies (e.g., interface methods,
function-typed variables, and imported functions/methods without
inline bodies).
name old time/op new time/op delta
Template 254ms ± 6% 251ms ± 6% -1.04% (p=0.000 n=487+488)
Unicode 128ms ± 7% 128ms ± 7% ~ (p=0.294 n=482+467)
GoTypes 862ms ± 5% 860ms ± 4% ~ (p=0.075 n=488+471)
Compiler 3.91s ± 4% 3.90s ± 4% -0.39% (p=0.000 n=468+473)
name old user-time/op new user-time/op delta
Template 339ms ±14% 336ms ±14% -1.02% (p=0.001 n=498+494)
Unicode 176ms ±18% 176ms ±25% ~ (p=0.940 n=491+499)
GoTypes 1.13s ± 8% 1.13s ± 9% ~ (p=0.157 n=496+493)
Compiler 5.24s ± 6% 5.21s ± 6% -0.57% (p=0.000 n=485+489)
name old alloc/op new alloc/op delta
Template 38.3MB ± 0% 37.3MB ± 0% -2.58% (p=0.000 n=499+497)
Unicode 29.1MB ± 0% 29.1MB ± 0% -0.03% (p=0.000 n=500+493)
GoTypes 116MB ± 0% 115MB ± 0% -0.65% (p=0.000 n=498+499)
Compiler 492MB ± 0% 487MB ± 0% -1.00% (p=0.000 n=497+498)
name old allocs/op new allocs/op delta
Template 364k ± 0% 360k ± 0% -1.15% (p=0.000 n=499+499)
Unicode 336k ± 0% 336k ± 0% -0.01% (p=0.000 n=500+493)
GoTypes 1.16M ± 0% 1.16M ± 0% -0.30% (p=0.000 n=499+499)
Compiler 4.54M ± 0% 4.51M ± 0% -0.58% (p=0.000 n=494+495)
Passes toolstash-check -gcflags=-dwarf=false. Changes DWARF output
because position information is now tracked more precisely for
function parameters.
Change-Id: Ib8077d70d564cc448c5e4290baceab3a4396d712
Reviewed-on: https://go-review.googlesource.com/108217
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Focus on "isfoo" funcs that take a *Node, and conver them to isFoo
methods instead. This makes for more idiomatic Go code, and also more
readable func names.
Found candidates with grep, and applied most changes with sed. The funcs
chosen were isgoconst, isnil, and isblank. All had the same signature,
func(*Node) bool.
While at it, camelCase the isliteral and iszero function names. Don't
move these to methods, as they are only used in the backend part of gc,
which might one day be split into a separate package.
Passes toolstash -cmp on std cmd.
Change-Id: I4df081b12d36c46c253167c8841c5a841f1c5a16
Reviewed-on: https://go-review.googlesource.com/105555
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Inl, Inldcl, and InlCost are only applicable to functions with bodies
that can be inlined, so pull them out into a separate Inline type to
make understanding them easier.
A side benefit is that we can check if a function can be inlined by
just checking if n.Func.Inl is non-nil, which simplifies handling of
empty function bodies.
While here, remove some unnecessary Curfn twiddling, and make imported
functions use Inl.Dcl instead of Func.Dcl for consistency for local
functions.
Passes toolstash-check.
Change-Id: Ifd4a80349d85d9e8e4484952b38ec4a63182e81f
Reviewed-on: https://go-review.googlesource.com/104756
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
When making a shallow copy of a node, various methods were used,
including calling nod(OXXX, nil, nil) and then overwriting it, or
"n1 := *n" and then using &n1.
Add a copy method instead, simplifying all of those and making them
consistent.
Passes toolstash -cmp on std cmd.
Change-Id: I3f3fc88bad708edc712bf6d87214cda4ddc43b01
Reviewed-on: https://go-review.googlesource.com/72710
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
We used to have three Sym flags for dealing with export/reexport:
Export, Package, and Exported.
Export and Package were used to distinguish whether a symbol is
exported or package-scope (i.e., mutually exclusive), except that for
local declarations Export served double-duty as tracking whether the
symbol had been added to exportlist.
Meanwhile, imported declarations that needed reexporting could be
added to exportlist multiple times, necessitating a flag to track
whether they'd already been written out by exporter.
Simplify all of these into a single OnExportList flag so that we can
ensure symbols on exportlist are present exactly once. Merge
reexportsym into exportsym so there's a single place where we append
to exportlist.
Code that used to set Exported to prevent a symbol from being exported
can now just set OnExportList before calling declare to prevent it
from even appearing on exportlist.
Lastly, drop the IsAlias check in exportsym: we call exportsym too
early for local symbols to detect if they're an alias, and we never
reexport aliases.
Passes toolstash-check.
Change-Id: Icdea3719105dc169fcd7651606589cd08b0a80ff
Reviewed-on: https://go-review.googlesource.com/103865
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Currently, we reexport any package-scope constant, function, type, or
variable declarations needed by an inlineable function body. However,
now that we have an early pass to walk inlineable function bodies
(golang.org/cl/74110), we can simplify the logic for finding these
declarations.
The binary export format supports writing out type declarations
in-place at their first use. Also, it always writes out constants by
value, so their declarations never need to be reexported.
Notably, we attempted this before (golang.org/cl/36170) and had to
revert it (golang.org/cl/45911). However, this was because while
writing out inline bodies, we could discover variable/function
dependencies. By collecting variable/function dependencies during
inlineable function discovery, we avoid this problem.
While here, get rid of isInlineable. We already typecheck inlineable
function bodies during inlFlood, so it's become a no-op. Just move the
comment explaining parameter numbering to its caller.
Change-Id: Ibbfaafce793733675d3a2ad98791758583055666
Reviewed-on: https://go-review.googlesource.com/103864
Reviewed-by: Robert Griesemer <gri@golang.org>
Rather than checking for each function whether the package supports
instrumentation, check once up front.
Relatedly, tweak the logic for preventing inlining calls to runtime
functions from instrumented packages. Previously, we simply disallowed
inlining runtime functions altogether when instrumenting. With this
CL, it's only disallowed from packages that are actually being
instrumented. That is, now intra-runtime calls can be inlined.
Updates #19054.
Change-Id: I88c97b48bf70193a8a3ee18d952dcb26b0369d55
Reviewed-on: https://go-review.googlesource.com/102815
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Eliminates an inconsistency between user functions and generated
functions.
Passes toolstash-check.
Change-Id: I946b511ca81d88a0024b5932cb50f3d8b9e808f4
Reviewed-on: https://go-review.googlesource.com/103863
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
The main thing is we now eagerly create the ODCLFUNC node for
closures, immediately cross-link them, and assign fields (e.g., Nbody,
Dcl, Parents, Marks) directly on the ODCLFUNC (previously they were
assigned on the OCLOSURE and later moved to the ODCLFUNC).
This allows us to set Curfn to the ODCLFUNC instead of the OCLOSURE,
which makes things more consistent with normal function declarations.
(Notably, this means Cvars now hang off the ODCLFUNC instead of the
OCLOSURE.)
Assignment of xfunc symbol names also now happens before typechecking
their body, which means debugging output now provides a more helpful
name than "<S>".
In golang.org/cl/66810, we changed "x := y" statements to avoid
creating false closure variables for x, but we still create them for
struct literals like "s{f: x}". Update comment in capturevars
accordingly.
More opportunity for cleanups still, but this makes some substantial
progress, IMO.
Passes toolstash-check.
Change-Id: I65a4efc91886e3dcd1000561348af88297775cd7
Reviewed-on: https://go-review.googlesource.com/100197
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
As a side effect of working on mid-stack inlining, we've fixed support
for inlining variadic functions. Might as well enable it.
Change-Id: I7f555f8b941969791db7eb598c0b49f6dc0820aa
Reviewed-on: https://go-review.googlesource.com/100456
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
These are always set to n.Isddd(), which is readily available within
mkinlcall.
Change-Id: I3d7fbc9dc19a40d6b905691c666eee9bcd031a00
Reviewed-on: https://go-review.googlesource.com/100455
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Add helper methods that validate n.Op and convert to/from the
appropriate type.
Notably, there was a lot of code in walk.go that thought setting
Etype=1 on an OADDR node affected escape analysis.
Passes toolstash-check.
TBR=marvin
Change-Id: Ieae7c67225c1459c9719f9e6a748a25b975cf758
Reviewed-on: https://go-review.googlesource.com/99535
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We accidentally overlooked needing to still visit Ninit for OIF
statements with constant conditions in golang.org/cl/96778.
Fixes#24120.
Change-Id: I5b341913065ff90e1163fb872b9e8d47e2a789d2
Reviewed-on: https://go-review.googlesource.com/97475
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently, the heap arena map is a single, large array that covers
every possible arena frame in the entire address space. This is
practical up to about 48 bits of address space with 64 MB arenas.
However, there are two problems with this:
1. mips64, ppc64, and s390x support full 64-bit address spaces (though
on Linux only s390x has kernel support for 64-bit address spaces).
On these platforms, it would be good to support these larger
address spaces.
2. On Windows, processes are charged for untouched memory, so for
processes with small heaps, the mostly-untouched 32 MB arena map
plus a 64 MB arena are significant overhead. Hence, it would be
good to reduce both the arena map size and the arena size, but with
a single-level arena, these are inversely proportional.
This CL adds support for a two-level arena map. Arena frame numbers
are now divided into arenaL1Bits of L1 index and arenaL2Bits of L2
index.
At the moment, arenaL1Bits is always 0, so we effectively have a
single level map. We do a few things so that this has no cost beyond
the current single-level map:
1. We embed the L2 array directly in mheap, so if there's a single
entry in the L2 array, the representation is identical to the
current representation and there's no extra level of indirection.
2. Hot code that accesses the arena map is structured so that it
optimizes to nearly the same machine code as it does currently.
3. We make some small tweaks to hot code paths and to the inliner
itself to keep some important functions inlined despite their
now-larger ASTs. In particular, this is necessary for
heapBitsForAddr and heapBits.next.
Possibly as a result of some of the tweaks, this actually slightly
improves the performance of the x/benchmarks garbage benchmark:
name old time/op new time/op delta
Garbage/benchmem-MB=64-12 2.28ms ± 1% 2.26ms ± 1% -1.07% (p=0.000 n=17+19)
(https://perf.golang.org/search?q=upload:20180223.2)
For #23900.
Change-Id: If5164e0961754f97eb9eca58f837f36d759505ff
Reviewed-on: https://go-review.googlesource.com/96779
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
The front-end dead code elimination is very simple. Currently, it just
looks for if statements with constant boolean conditions. Its main
purpose is to reduce load on the compiler and shrink code before
inlining computes hairiness.
This CL teaches front-end dead code elimination about short-circuiting
boolean expressions && and ||, since they're essentially the same as
if statements.
This also teaches the inliner that the constant 'if' form left behind
by deadcode is free.
These changes will help with runtime modifications in the next CL that
would otherwise inhibit inlining in some hot code paths. Currently,
however, they have no significant impact on benchmarks.
Change-Id: I886203b3c4acdbfef08148fddd7f3a7af5afc7c1
Reviewed-on: https://go-review.googlesource.com/96778
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
recover determines whether it's being called by a deferred frame by
matching its caller's argument frame pointer with the one recorded in
the panic object. That means its caller needs a valid and unique
argument frame pointer, so it must not be inlined.
With this fix, test/recover.go passes with -l=4.
Fixes#23557.
Change-Id: I1f32a624c49e387cfc67893a0829bb248d69c3d4
Reviewed-on: https://go-review.googlesource.com/90035
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Change the compiler's DWARF inline info generation to be more careful
about producing consistent instances of abstract function DIEs. The
new strategy is to insure that the only params/variables created in an
abstract subprogram DIE are those corresponding to declarations in the
original pre-inlining version of the code. If a concrete subprogram
winds up with other vars as part of the compilation process (return
temps, for example, or scalars generated by splitting a structure into
pieces) these are emitted as regular param/variable DIEs instead of
concrete DIEs.
The linker dwarf test now has a couple of new testpoints that include
checks to make sure that all abstract DIE references are
sane/resolvable; this will help catch similar problems in the future.
Fixes#23046.
Change-Id: I9b0030da8673fbb80b7ad50461fcf8c6ac823a37
Reviewed-on: https://go-review.googlesource.com/83675
Run-TryBot: Than McIntosh <thanm@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We can't currently inline functions that contain closures anyway, so
just delete this budgeting code for now. Re-enable once we can (if
ever) inline functions with nested closures.
Updates #15561.
Fixes#23093.
Change-Id: Idc5f8e042ccfcc8921022e58d3843719d4ab821e
Reviewed-on: https://go-review.googlesource.com/83538
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Compiler and linker changes to support DWARF inlined instances,
see https://go.googlesource.com/proposal/+/HEAD/design/22080-dwarf-inlining.md
for design details.
This functionality is gated via the cmd/compile option -gendwarfinl=N,
where N={0,1,2}, where a value of 0 disables dwarf inline generation,
a value of 1 turns on dwarf generation without tracking of formal/local
vars from inlined routines, and a value of 2 enables inlines with
variable tracking.
Updates #22080
Change-Id: I69309b3b815d9fed04aebddc0b8d33d0dbbfad6e
Reviewed-on: https://go-review.googlesource.com/75550
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
CL 65071 enabled inlining for local closures with no captures.
To determine safety of inlining a call sites, we check whether the
variable holding the closure has any assignments after its original
definition.
Unfortunately, that check did not catch OAS2MAPR and OAS2DOTTYPE,
leading to incorrect inlining when a variable holding a closure was
subsequently reassigned through a type conversion or a 2-valued map
access.
There was another more subtle issue wherein reassignment check would
always return a false positive for closure calls inside other
closures. This was caused by the Name.Curfn field of local variables
pointing to the OCLOSURE node instead of the corresponding ODCLFUNC,
which resulted in reassigned walking an empty Nbody and thus never
seeing any reassignments.
This CL fixes these oversights and adds many more tests for closure
inlining which ensure not only that inlining triggers but also the
correctness of the resulting code.
Updates #15561
Change-Id: I74bdae849c4ecfa328546d6d62b512e8d54d04ce
Reviewed-on: https://go-review.googlesource.com/75770
Reviewed-by: Hugues Bruant <hugues.bruant@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Previously, anytime we exported a function or method declaration
(which includes methods for every type transitively exported), we
included the inline function bodies, if any. However, in many cases,
it's impossible (or at least very unlikely) for the importing package
to call the method.
For example:
package p
type T int
func (t T) M() { t.u() }
func (t T) u() {}
func (t T) v() {}
T.M and T.u are inlineable, and they're both reachable through calls
to T.M, which is exported. However, t.v is also inlineable, but cannot
be reached.
Exception: if p.T is embedded in another type q.U, p.T.v will be
promoted to q.U.v, and the generated wrapper function could have
inlined the call to p.T.v. However, in practice, this doesn't happen,
and a missed inlining opportunity doesn't affect correctness.
To implement this, this CL introduces an extra flood fill pass before
exporting to mark inline bodies that are actually reachable, so the
exporter can skip over methods like t.v.
This reduces Kubernetes build time (as measured by "time go build -a
k8s.io/kubernetes/cmd/...") on an HP Z620 measurably:
== before ==
real 0m44.658s
user 11m19.136s
sys 0m53.844s
== after ==
real 0m41.702s
user 10m29.732s
sys 0m50.908s
It also significantly cuts down the cost of enabling mid-stack
inlining (-l=4):
== before (-l=4) ==
real 1m19.236s
user 20m6.528s
sys 1m17.328s
== after (-l=4) ==
real 0m59.100s
user 13m12.808s
sys 0m58.776s
Updates #19348.
Change-Id: Iade58233ca42af823a1630517a53848b5d3c7a7e
Reviewed-on: https://go-review.googlesource.com/74110
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Eliminates lots of ad hoc code for recognizing the same thing in
different ways.
Passes toolstash-check.
Change-Id: Ic0bb005308e96331b4ef30f455b860e476725b61
Reviewed-on: https://go-review.googlesource.com/73190
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently, benchmarking compile performance under -l=4 is confounded
by -l=2 enabling eager typechecking of unused inline function bodies
for debugging. This isn't logically an "inlining aggressiveness"
level, so instead move this logic under the -d umbrella flag.
Change-Id: I713f68952efbe25b6941d3ebc2f3707ccbbd6240
Reviewed-on: https://go-review.googlesource.com/72253
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Calls to a closure held in a local, non-escaping,
variable can be inlined, provided the closure body
can be inlined and the variable is never written to.
The current implementation has the following limitations:
- closures with captured variables are not inlined because
doing so naively triggers invariant violation in the SSA
phase
- re-assignment check is currently approximated by checking
the Addrtaken property of the variable which should be safe
but may miss optimization opportunities if the address is
not used for a write before the invocation
Updates #15561
Change-Id: I508cad5d28f027bd7e933b1f793c14dcfef8b5a1
Reviewed-on: https://go-review.googlesource.com/65071
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Hugues Bruant <hugues.bruant@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
mkinlcall1 already guards against recursively inlining functions into
themselves, so there's no need to clear and restore fn.Func.Inl during
recursive inlining.
Passes toolstash-check.
Change-Id: I8bf0c8dea8788d94d3ea5670610b4acb1d26d2fb
Reviewed-on: https://go-review.googlesource.com/69310
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
We've supported inlining methods called as functions for a while now.
Change-Id: I53fba426e45f91d65a38f00456c2ae1527372b50
Reviewed-on: https://go-review.googlesource.com/69530
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
When debugging inliner with -m -m print cost of complex functions,
instead of simple "function too complex". This helps to understand,
how close to inlining is this particular function.
Change-Id: I6871f69b5b914d23fd0b43a24d7c6fc928f4b716
Reviewed-on: https://go-review.googlesource.com/63330
Run-TryBot: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Previously, after inlining a call, we made a second pass to rewrite
the AST's position information to record the inlined stack frame. The
call arguments were part of this AST, but it would be incorrect to
rewrite them too, so extra effort was made to temporarily remove them
while the position rewriting was done.
However, this extra logic was only done for regular arguments: it was
not done for receiver arguments. Consequently if m was inlined in
"f().m(g(), h())", g and h would have correct call frames, but f would
appear to be called by m.
The fix taken by this CL is to merge setpos into inlsubst and only
rewrite position information for nodes that were actually copied from
the original function AST body. As a side benefit, this eliminates an
extra AST pass and some AST walking code.
Fixes#21879.
Change-Id: I22b25c208313fc25c358d3a2eebfc9b012400084
Reviewed-on: https://go-review.googlesource.com/64470
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Arguments to inlined calls are hidden from setPos as follows:
args := as.Rlist
as.Rlist.Set(nil)
// setPos...
as.Rlist.Set(args.Slice())
Previously, this code had no effect since the value of as was
overwritten by the assignment in the retvars loop.
Fixes#19799.
Change-Id: Iaf97259f82fdba8b236136337cc42b2774c7fef5
Reviewed-on: https://go-review.googlesource.com/44351
Run-TryBot: David Lazar <lazard@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Node.Used was written to from the backend
concurrently with reads of Node.Class
for the same ONAME Nodes.
I do not know why it was not failing consistently
under the race detector, but it is a race.
This is likely also a problem with Node.HasVal and Node.HasOpt.
They will be handled in a separate CL.
Fix Used by moving it to gc.Name and making it a separate bool.
There was one non-Name use of Used, marking OLABELs as used.
That is no longer needed, now that goto and label checking
happens early in the front end.
Leave the getters and setters in place,
to ease changing the representation in the future
(or changing to an interface!).
Updates #20144
Change-Id: I9bec7c6d33dcb129a4cfa9d338462ea33087f9f7
Reviewed-on: https://go-review.googlesource.com/42015
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The inliner's ishairy passes a budget and a reason down through the
walk. Lift these into a visitor object and turn ishairy and its
helpers into methods.
This will make it easy to add more state.
Change-Id: Ic6ae246e1affd67ed283c3205f9595ae33e22215
Reviewed-on: https://go-review.googlesource.com/41151
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
