If the function referenced by a closure expression is incorporated
into a static init, be sure to mark it as non-hidden, since otherwise
it will be live but no longer reachable from the init func, hence it
will be skipped during escape analysis, which can lead to
miscompilations.
Fixes#59680.
Change-Id: Ib858aee296efcc0b7655d25c23ab8a6a8dbdc5f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/492135
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
[This is a roll-forward of CL 484859, this time including a fix for
issue #59709. The call to do dead function marking was taking place in
the wrong spot, causing it to run more than once if generics were
instantiated.]
This patch generalizes the code in the inliner that marks unreferenced
hidden closure functions as dead. Rather than doing the marking on the
fly (previous approach), this new approach does a single pass at the
end of inlining, which catches more dead functions.
Change-Id: I0e079ad755c21295477201acbd7e1a732a98fffd
Reviewed-on: https://go-review.googlesource.com/c/go/+/492016
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Previously, type arguments could only be inferred for generic
functions in call expressions, whereas with the reverse type inference
proposal they can now be inferred in assignment contexts too. As a
consequence, we now need to check Info.Instances to find the inferred
type for more cases now.
Updates #59338.
Fixes#59955.
Change-Id: I9b6465395869459c2387d0424febe7337b28b90e
Reviewed-on: https://go-review.googlesource.com/c/go/+/492455
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
This CL enhances the tighten pass. Previously if a value has memory arg,
then the tighten pass won't move it, actually if the memory state is
consistent among definition and use block, we can move the value. This
CL optimizes this case. This is useful for the following situation:
b1:
x = load(...mem)
if(...) goto b2 else b3
b2:
use(x)
b3:
some_op_not_use_x
For the micro-benchmark mentioned in #56620, the performance improvement
is about 15%.
There's no noticeable performance change in the go1 benchmark.
Fixes#56620
Change-Id: I9b152754f27231f583a6995fc7cd8472aa7d390c
Reviewed-on: https://go-review.googlesource.com/c/go/+/458755
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Put zero-sized data symbols at same address as runtime.zerobase,
so zero-sized global variables have the same address as zero-sized
allocations.
Change-Id: Ib3145dc1b663a9794dfabc0e6abd2384960f2c49
Reviewed-on: https://go-review.googlesource.com/c/go/+/490435
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
The current definitions of StackLimit and StackGuard only indirectly
specify the NOSPLIT stack limit and duplicate a literal constant
(928). Currently, they define the stack guard delta, and from there
compute the NOSPLIT limit.
Rationalize these by defining a new constant, abi.StackNosplitBase,
which consolidates and directly specifies the NOSPLIT stack limit (in
the default case). From this we then compute the stack guard delta,
inverting the relationship between these two constants. While we're
here, we rename StackLimit to StackNosplit to make it clearer what's
being limited.
This change does not affect the values of these constants in the
default configuration. It does slightly change how
StackGuardMultiplier values other than 1 affect the constants, but
this multiplier is a pretty rough heuristic anyway.
before after
stackNosplit 800 800
_StackGuard 928 928
stackNosplit -race 1728 1600
_StackGuard -race 1856 1728
For #59670.
Change-Id: Ia94094c5e47897e7c088d24b4a5e33f5c2768db5
Reviewed-on: https://go-review.googlesource.com/c/go/+/486976
Auto-Submit: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This reverts commit CL 486380.
Submitted out of order and breaks bootstrap.
Change-Id: I67bd225094b5c9713b97f70feba04d2c99b7da76
Reviewed-on: https://go-review.googlesource.com/c/go/+/486916
Reviewed-by: David Chase <drchase@google.com>
TryBot-Bypass: Austin Clements <austin@google.com>
The current definitions of StackLimit and StackGuard only indirectly
specify the NOSPLIT stack limit and duplicate a literal constant
(928). Currently, they define the stack guard delta, and from there
compute the NOSPLIT limit.
Rationalize these by defining a new constant, abi.StackNosplitBase,
which consolidates and directly specifies the NOSPLIT stack limit (in
the default case). From this we then compute the stack guard delta,
inverting the relationship between these two constants. While we're
here, we rename StackLimit to StackNosplit to make it clearer what's
being limited.
This change does not affect the values of these constants in the
default configuration. It does slightly change how
StackGuardMultiplier values other than 1 affect the constants, but
this multiplier is a pretty rough heuristic anyway.
before after
stackNosplit 800 800
_StackGuard 928 928
stackNosplit -race 1728 1600
_StackGuard -race 1856 1728
For #59670.
Change-Id: Ibe20825ebe0076bbd7b0b7501177b16c9dbcb79e
Reviewed-on: https://go-review.googlesource.com/c/go/+/486380
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Marking variables in erroneous variable declarations as used is
convenient for tests but doesn't necessarily hide follow-on errors
in real code: either the variable is not supposed to be declared in
the first place and then we should get an error if it is not used,
or it is there because it is intended to be used, and the we expect
an error it if is not used.
This brings types2 closer to go/types.
Change-Id: If7ee1298fc770f7ad0cefe7e968533fd50ec2343
Reviewed-on: https://go-review.googlesource.com/c/go/+/486175
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Add more rules to ensure that order doesn't matter.
Add memequal 0 rule.
Try to use a constant argument to memequal when one is available.
Fixes#59684
Change-Id: I36e85ffbd949396ed700ed6e8ec2bc3ae013f5d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/485535
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This reverts commit http://go.dev/cl//484859
Reason for revert: causes linker errors in a number of google-internal tests.
Change-Id: I322252f784a46d2b1d447ebcdca86ce14bc0cc91
Reviewed-on: https://go-review.googlesource.com/c/go/+/485755
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
[This is a roll-forward of CL 479095, which was reverted due to a bad
interaction between inlining and escape analysis, then later fixed
fist with an attempt in CL 482355, then again in 484859 .]
Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:
- If the closure contains a disallowed operation (e.g., a defer), then
the outer function can't be inlined. It makes sense that the
*closure* can't be inlined in this case, but it doesn't make sense
to punish the function that constructs the closure.
- The hairiness of the closure counts against the inlining budget of
the outer function. Since we currently copy the closure body when
inlining the outer function, this makes sense from the perspective
of export data size and binary size, but ultimately doesn't make
much sense from the perspective of what should be inlineable.
- Since the inliner walks into every closure created by an outer
function in addition to starting a walk at every closure, this adds
an n^2 factor to inlinability analysis.
This CL simply drops this behavior.
In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).
This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.
The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:
│ before │ after │
│ bytes │ bytes vs base │
Go/binary 15.12Mi ± 0% 15.14Mi ± 0% +0.16% (n=1)
Go/text 5.220Mi ± 0% 5.237Mi ± 0% +0.32% (n=1)
Compile/binary 22.92Mi ± 0% 22.94Mi ± 0% +0.07% (n=1)
Compile/text 8.428Mi ± 0% 8.435Mi ± 0% +0.08% (n=1)
Updates #56102.
Change-Id: I6e938d596992ffb473cf51e7e598f372ce08deb0
Reviewed-on: https://go-review.googlesource.com/c/go/+/484860
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This patch generalizes the code in the inliner that marks unreferenced
hidden closure functions as dead. Rather than doing the marking on the
fly (previous approach), this new approach does a single pass at the
end of inlining, which catches more dead functions.
Fixes#59638.
Updates #59404.
Updates #59547.
Change-Id: I54fd63e9e37c9123b08a3e7def7d1989919bba91
Reviewed-on: https://go-review.googlesource.com/c/go/+/484859
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
CL 399694 added constant-fold switch early in compilation. So function:
func f() string {
switch intSize {
case 32:
return "32"
case 64:
return "64"
default:
panic("unreachable")
}
}
will be constant-fold to:
func f() string {
switch intSize {
case 64:
return "64"
}
}
When this function get inlined, there is a check whether we can delay
declaring the result parameter until the "return" statement. For the
original function, we can't delay the result, because there's more than
one return statement. However, the constant-fold one can, because
there's on one return statement in the body now. The result parameter
~R0 ends up declaring inside the switch statement scope.
Now, when walking the switch statement, it's re-written into if-else
statement. Without typecheck.EvalConst, the if condition "if 64 == 64"
is passed as-is to the ssa generation pass. Because "64 == 64" is not a
constant, the ssagen creates normal blocks for branching the results.
This confuses the liveness analysis, because ~R0 is only live inside the
if block. With typecheck.EvalConst, "64 == 64" is evaluated to "true",
so ssagen can branch the result without emitting conditional blocks.
Instead, the constant-fold can be re-written as:
switch {
case true:
// Body
}
So it does not depend on the delay results check during inlining. Adding
a test, which will fail when typecheck.EvalConst is removed, so we can
do the cleanup without breaking things.
Change-Id: I638730bb147140de84260653741431b807ff2f15
Reviewed-on: https://go-review.googlesource.com/c/go/+/484316
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Static init inliner is using typecheck.EvalConst to handle string
concatenation expressions. But static init inliner may reveal constant
expressions after substitution, and the compiler needs to evaluate those
expressions in non-constant semantic. Using typecheck.EvalConst, which
always evaluates expressions in constant semantic, is not the right
choice.
For safety, this CL fold the logic to handle string concatenation to
static init inliner, so there won't be regression in handling constant
expressions in non-constant semantic. And also, future CL can simplify
typecheck.EvalConst logic.
Updates #58293
Updates #58339Fixes#58439
Change-Id: I74068d99c245938e576afe9460cbd2b39677bbff
Reviewed-on: https://go-review.googlesource.com/c/go/+/466277
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
(This is a retry of CL 462035 which was reverted at 474976.
The only change from that CL is the aix fix SRODATA->SNOPTRDATA
at inittask.go:141)
As described here:
https://github.com/golang/go/issues/31636#issuecomment-493271830
"Find the lexically earliest package that is not initialized yet,
but has had all its dependencies initialized, initialize that package,
and repeat."
Simplify the runtime a bit, by just computing the ordering required
in the linker and giving a list to the runtime.
Update #31636Fixes#57411
RELNOTE=yes
Change-Id: I28c09451d6aa677d7394c179d23c2c02c503fc56
Reviewed-on: https://go-review.googlesource.com/c/go/+/478916
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This reverts commit http://go.dev/cl/c/482356.
Reason for revert: Reverting this change again, since it is causing additional failures in google-internal testing.
Change-Id: I9234946f62e5bb18c2f873a65e8b298d04af0809
Reviewed-on: https://go-review.googlesource.com/c/go/+/484735
Reviewed-by: Florian Zenker <floriank@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Auto-Submit: Than McIntosh <thanm@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Range statement will mutate the key and value, so we should treat them as reassigned.
Fixes#59572
Change-Id: I9c6b67d938760a0c6a1d9739f2737c67af4a3a10
Reviewed-on: https://go-review.googlesource.com/c/go/+/483855
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Fixes#58141
Co-authored-by: Richard Musiol <neelance@gmail.com>
Co-authored-by: Achille Roussel <achille.roussel@gmail.com>
Co-authored-by: Julien Fabre <ju.pryz@gmail.com>
Co-authored-by: Evan Phoenix <evan@phx.io>
Change-Id: I49b66946acc90fdf09ed9223096bfec9a1e5b923
Reviewed-on: https://go-review.googlesource.com/c/go/+/479627
Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@golang.org>
For now, only apply the rule if either of arguments are constants. That
would catch a lot of real user code, without slowing down the compiler
with code generated for string comparison (experience in CL 410336).
Updates #57959Fixes#45928
Change-Id: Ie2e830d6d0d71cda3947818b22c2775bd94f7971
Reviewed-on: https://go-review.googlesource.com/c/go/+/483359
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
So iterators that are in progress can know entries have been deleted and
terminate the iterator properly.
Update #55002
Update #56351Fixes#59411
Change-Id: I924f16a00fe4ed6564f730a677348a6011d3fb67
Reviewed-on: https://go-review.googlesource.com/c/go/+/481935
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Use the type of the store for the byteswap, not the type of the
store's value argument.
Normally when we're storing a 16-bit value, the value being stored is
also typed as 16 bits. But sometimes it is typed as something smaller,
usually because it is the result of an upcast from a smaller value,
and that upcast needs no instructions.
If the type of the store's arg is thinner than the type being stored,
and the byteswap'd value uses that thinner type, and the byteswap'd
value needs to be spilled & restored, that spill/restore happens using
the thinner type, which causes us to lose some of the top bits of the
value.
Fixes#59367
Change-Id: If6ce1e8a76f18bf8e9d79871b6caa438bc3cce4d
Reviewed-on: https://go-review.googlesource.com/c/go/+/481395
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
[This is a roll-forward of CL 479095, which was reverted due to a bad
interaction between inlining and escape analysis since fixed in CL 482355.]
Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:
- If the closure contains a disallowed operation (e.g., a defer), then
the outer function can't be inlined. It makes sense that the
*closure* can't be inlined in this case, but it doesn't make sense
to punish the function that constructs the closure.
- The hairiness of the closure counts against the inlining budget of
the outer function. Since we currently copy the closure body when
inlining the outer function, this makes sense from the perspective
of export data size and binary size, but ultimately doesn't make
much sense from the perspective of what should be inlineable.
- Since the inliner walks into every closure created by an outer
function in addition to starting a walk at every closure, this adds
an n^2 factor to inlinability analysis.
This CL simply drops this behavior.
In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).
This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.
The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:
│ before │ after │
│ bytes │ bytes vs base │
Go/binary 15.12Mi ± 0% 15.14Mi ± 0% +0.16% (n=1)
Go/text 5.220Mi ± 0% 5.237Mi ± 0% +0.32% (n=1)
Compile/binary 22.92Mi ± 0% 22.94Mi ± 0% +0.07% (n=1)
Compile/text 8.428Mi ± 0% 8.435Mi ± 0% +0.08% (n=1)
Updates #56102.
Change-Id: I1f4fc96c71609c8feb59fecdb92b69ba7e3b5b41
Reviewed-on: https://go-review.googlesource.com/c/go/+/482356
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When a closure is inlined, it may contain other hidden closures, which
the inliner will duplicate, rendering the original nested closures as
unreachable. Because they are unreachable, they don't get processed in
escape analysis, meaning that go/defer statements don't get rewritten,
which can then in turn trigger errors in walk. This patch looks for
nested hidden closures and marks them as dead, so that they can be
skipped later on in the compilation flow. NB: if during escape
analysis we rediscover a hidden closure (due to an explicit reference)
that was previously marked dead, revive it at that point.
Fixes#59404.
Change-Id: I76db1e9cf1ee38bd1147aeae823f916dbbbf081b
Reviewed-on: https://go-review.googlesource.com/c/go/+/482355
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Currently, the prove pass can get knowledge from some specific logic
operators only before the CFG is explored, which means that the bounds
information of the branch will be ignored.
This CL updates the facts table by the logic operators in every
branch. Combined with the branch information, this will be helpful for
BCE in some circumstances.
Fixes#57243
Change-Id: I0bd164f1b47804ccfc37879abe9788740b016fd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/419555
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Eric Fang <eric.fang@arm.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Unified IR already records the correct type for them.
Fixes#59378
Change-Id: I275c45b48f67bde55c8e2079d60b5868d0acde7f
Reviewed-on: https://go-review.googlesource.com/c/go/+/481555
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Casting to a *uintptr is not ok if there isn't at least 8 bytes of
data backing that pointer (on 64-bit archs).
So although we end up making a slice of 0 length with that pointer,
the cast itself doesn't know that.
Instead, bail early if the result is going to be 0 length.
Fixes#59334
Change-Id: Id3c0e09d341d838835c0382cccfb0f71dc3dc7e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/480575
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Currently, when the inliner is determining if a function is
inlineable, it descends into the bodies of closures constructed by
that function. This has several unfortunate consequences:
- If the closure contains a disallowed operation (e.g., a defer), then
the outer function can't be inlined. It makes sense that the
*closure* can't be inlined in this case, but it doesn't make sense
to punish the function that constructs the closure.
- The hairiness of the closure counts against the inlining budget of
the outer function. Since we currently copy the closure body when
inlining the outer function, this makes sense from the perspective
of export data size and binary size, but ultimately doesn't make
much sense from the perspective of what should be inlineable.
- Since the inliner walks into every closure created by an outer
function in addition to starting a walk at every closure, this adds
an n^2 factor to inlinability analysis.
This CL simply drops this behavior.
In std, this makes 57 more functions inlinable, and disallows inlining
for 10 (due to the basic instability of our bottom-up inlining
approach), for an net increase of 47 inlinable functions (+0.6%).
This will help significantly with the performance of the functions to
be added for #56102, which have a somewhat complicated nesting of
closures with a performance-critical fast path.
The downside of this seems to be a potential increase in export data
and text size, but the practical impact of this seems to be
negligible:
│ before │ after │
│ bytes │ bytes vs base │
Go/binary 15.12Mi ± 0% 15.14Mi ± 0% +0.16% (n=1)
Go/text 5.220Mi ± 0% 5.237Mi ± 0% +0.32% (n=1)
Compile/binary 22.92Mi ± 0% 22.94Mi ± 0% +0.07% (n=1)
Compile/text 8.428Mi ± 0% 8.435Mi ± 0% +0.08% (n=1)
Change-Id: Ie9e38104fed5689a94c368288653fd7cb4b7a35e
Reviewed-on: https://go-review.googlesource.com/c/go/+/479095
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
The cast is proceeded by a bounds check. If the bounds check passes
then we know the pointer in the slice is non-nil.
... except casts to pointers of 0-sized arrays. They are strange, as
the bounds check can pass for a nil input.
Change-Id: Ic01cf4a82d59fbe3071d4b271c94efca9cafaec1
Reviewed-on: https://go-review.googlesource.com/c/go/+/479335
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Auto-Submit: Keith Randall <khr@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Change the Checker.use/useLHS functions to report if all "used"
expressions evaluated without error. Use that information to
control whether to report an assignment mismatch error or not.
This will reduce the number of errors reported per assignment,
where the assignment mismatch is only one of the errors.
Change-Id: Ia0fc3203253b002e4e1d5759d8d5644999af6884
Reviewed-on: https://go-review.googlesource.com/c/go/+/478756
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
For #55242
Change-Id: I092b1881623ea997b178d038c0afd10cd5bca937
Reviewed-on: https://go-review.googlesource.com/c/go/+/479898
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
unsafe.SliceData can return pointers which are nil. That function gets
lowered to the SSA OpSlicePtr, which the compiler assumes is non-nil.
This used to be the case as OpSlicePtr was only used in situations
where the bounds check already passed. But with unsafe.SliceData that
is no longer the case.
There are situations where we know it is nil. Use Bounded() to
indicate that.
I looked through all the uses of OSPTR and added SetBounded where it
made sense. Most OSPTR results are passed directly to runtime calls
(e.g. memmove), so even if we know they are non-nil that info isn't
helpful.
Fixes#59293
Change-Id: I437a15330db48e0082acfb1f89caf8c56723fc51
Reviewed-on: https://go-review.googlesource.com/c/go/+/479896
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
As with changes in prior CLs, we don't suppress legitimate
"declared but not used" errors anymore simply because the
respective variables are used in incorrect assignments,
unrelated to the variables in question.
Adjust several (ancient) tests accordingly.
Change-Id: I5826393264d9d8085c64777a330d4efeb735dd2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/478716
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
This CL re-introduces useLHS because we don't want to suppress
correct "declared but not used" errors for variables that only
appear on the LHS of an assignment (using Checker.use would mark
them as used).
This CL also adjusts a couple of places where types2 differed
from go/types (and suppressed valid "declared and not used"
errors). Now those errors are surfaced. Adjusted a handful of
tests accordingly.
Change-Id: Ia555139a05049887aeeec9e5221b1f41432c1a57
Reviewed-on: https://go-review.googlesource.com/c/go/+/478635
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Griesemer <gri@google.com>
Don't say "array length must be integer" if it is in fact an integer.
Fixes#59209
Change-Id: If60b93a0418f5837ac334412d3838eec25eeb855
Reviewed-on: https://go-review.googlesource.com/c/go/+/479115
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
In the Sizes API, recognize an overflow (to a negative value) as a
consequence of an oversize value, and specify as such in the API.
Adjust the various size computations to take overflow into account.
Recognize a negative size or offset as an error and report it rather
than panicking.
Use the same protocol for results provided by the default (StdSizes)
and external Sizes implementations.
Add a new error code TypeTooLarge for the new errors.
Fixes#59190.
Fixes#59207.
Change-Id: I8c33a9e69932760275100112dde627289ac7695b
Reviewed-on: https://go-review.googlesource.com/c/go/+/478919
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Under the right conditions we can optimize cmp comparisons to cmn
comparisons, such as:
func foo(a, b int) int {
var c int
if a + b < 0 {
c = 1
}
return c
}
Previously it's compiled as:
ADD R1, R0, R1
CMP $0, R1
CSET LT, R0
With this CL it's compiled as:
CMN R1, R0
CSET MI, R0
Here we need to pay attention to the overflow situation of a+b, the MI
flag means N==1, which doesn't honor the overflow flag V, its value
depends only on the sign of the result. So it has the same semantic of
the Go code, so it's correct.
Similarly, this CL also optimizes the case of >= comparison
using the PL conditional flag.
Change-Id: I47179faba5b30cca84ea69bafa2ad5241bf6dfba
Reviewed-on: https://go-review.googlesource.com/c/go/+/476116
Run-TryBot: Eric Fang <eric.fang@arm.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
For #59169
Change-Id: Id72ad9fe8b6e1d7cf64f972520ae8858f70c025a
Reviewed-on: https://go-review.googlesource.com/c/go/+/478217
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Add the following common local transformations
(t + x) - (t + y) == x - y
(t + x) - (y + t) == x - y
(x + t) - (y + t) == x - y
(x + t) - (t + y) == x - y
(x - t) + (t + y) == x + y
(x - t) + (y + t) == x + y
The compiler itself matches such patterns many times. This also aligns with other popular compilers.
Fixes#59111
Change-Id: Ibdfdb414782f8fcaa20b84ac5d43d0d9ae2c7b60
GitHub-Last-Rev: 1aad82e62e
GitHub-Pull-Request: golang/go#59119
Reviewed-on: https://go-review.googlesource.com/c/go/+/477555
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
This CL add support for instrinsifying the TrialingZeros{8,32,64}
functions for 386 architecture. We need handle the case when the input
is 0, which could lead to undefined output from the BSFL instruction.
Next CL will remove the assembly code in runtime/internal/sys package.
Change-Id: Ic168edf68e81bf69a536102100fdd3f56f0f4a1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/475735
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Wayne Zuo <wdvxdr@golangcn.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This patch changes the relative order of "CanInline" and "InlineCalls"
operations within the inliner for clumps of functions corresponding to
strongly connected components in the call graph. This helps increase
the amount of inlining within SCCs, particularly in Go's runtime
package, which has a couple of very large SCCs.
For a given SCC of the form { fn1, fn2, ... fnk }, the inliner would
(prior to this point) walk through the list of functions and for each
function first compute inlinability ("CanInline") and then perform
inlining ("InlineCalls"). This meant that if there was an inlinable
call from fn3 to fn4 (for example), this call would never be inlined,
since at the point fn3 was visited, we would not have computed
inlinability for fn4.
We now do inlinability analysis for all functions in an SCC first,
then do actual inlining for everything. This results in 47 additional
inlines in the Go runtime package (a fairly modest increase
percentage-wise of 0.6%).
Updates #58905.
Change-Id: I48dbb1ca16f0b12f256d9eeba8cf7f3e6dd853cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/474955
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
This reverts commit ce2a609909.
aka CL 462035
Reason for revert: this CL is causing some problems in some internal Google programs.
Change-Id: I4476b8d8d2c3d7b5703d1d85c93baebb4b4e5d26
Reviewed-on: https://go-review.googlesource.com/c/go/+/474976
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
