This CL implements the remainder of the zero-copy string->[]byte
conversion optimization initially attempted in go.dev/cl/520395, but
fixes the tracking of mutations due to ODEREF/ODOTPTR assignments, and
adds more comprehensive tests that I should have included originally.
However, this CL also keeps it behind the -d=zerocopy flag. The next
CL will enable it by default (for easier rollback).
Updates #2205.
Change-Id: Ic330260099ead27fc00e2680a59c6ff23cb63c2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/520599
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Add some machinery to support computing function "properties" for use
in driving inlining heuristics, and a unit testing framework to check
to see if the property computations are correct for a given set of
canned Go source files. This CL is mainly the analysis skeleton and a
testing framework; the code to compute the actual props will arrive in
a later patch.
Updates #61502.
Change-Id: I7970b64f713d17d7fdd7e8e9ccc7d9b0490571bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/511557
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL is originally based on CL 484838 from rajbarik@uber.com.
Add a new PGO-based devirtualize pass. This pass conditionally
devirtualizes interface calls for the hottest callee. That is, it
performs a transformation like:
type Iface interface {
Foo()
}
type Concrete struct{}
func (Concrete) Foo() {}
func foo(i Iface) {
i.Foo()
}
to:
func foo(i Iface) {
if c, ok := i.(Concrete); ok {
c.Foo()
} else {
i.Foo()
}
}
The primary benefit of this transformation is enabling inlining of the
direct calls.
Today this change has no impact on the escape behavior, as the fallback
interface always forces an escape. But improving escape analysis to take
advantage of this is an area of potential work.
This CL is the bare minimum of a devirtualization implementation. There
are still numerous limitations:
* Callees not directly referenced in the current package can be missed
(even if they are in the transitive dependences).
* Callees not in the transitive dependencies of the current package are
missed.
* Only interface method calls are supported, not other indirect function
calls.
* Multiple calls to compatible interfaces on the same line cannot be
distinguished and will use the same callee target.
* Callees that only partially implement an interface (they are embedded
in another type that completes the interface) cannot be devirtualized.
* Others, mentioned in TODOs.
Fixes#59959
Change-Id: I8bedb516139695ee4069650b099d05957b7ce5ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/492436
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
We will soon have PGO specialization. It doesn't make sense for the
debug flag to have inline in the name, so rename it to pgodebug.
pgoinline is now a flag that can be used to disable PGO inlining.
Devirtualization will have a similar debug flag.
For #59959.
Change-Id: I9770ff1f0d132dfa3cd417018a887a1bd5555bba
Reviewed-on: https://go-review.googlesource.com/c/go/+/494716
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Delete the "InlineSCCOnePass" debugging flag and the inliner fallback
code that kicks in if it is used. The change it was intended to guard
has been working on tip for some time, no need for the fallback any
more.
Updates #58905.
Change-Id: I2e1dbc7640902d9402213db5ad338be03deb96c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/492015
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
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>
Remove the compiler's "-wrapglobalmapinit" flag; it is potentially
confusing for users and isn't appropriate as a top level flag. Move
the enable/disable control to the "wrapglobalmapctl" debug flag
(values: 0 on by default, 1 disabled, 2 stress mode). No other changes
to compiler functionality.
Change-Id: I0d120eaf90ee34e29d5032889e673d42fe99e5dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/475035
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Adds:
GOEXPERIMENT=loopvar (expected way of invoking)
-d=loopvar={-1,0,1,2,11,12} (for per-package control and/or logging)
-d=loopvarhash=... (for hash debugging)
loopvar=11,12 are for testing, benchmarking, and debugging.
If enabled,for loops of the form `for x,y := range thing`, if x and/or
y are addressed or captured by a closure, are transformed by renaming
x/y to a temporary and prepending an assignment to the body of the
loop x := tmp_x. This changes the loop semantics by making each
iteration's instance of x be distinct from the others (currently they
are all aliased, and when this matters, it is almost always a bug).
3-range with captured iteration variables are also transformed,
though it is a more complex transformation.
"Optimized" to do a simpler transformation for
3-clause for where the increment is empty.
(Prior optimization of address-taking under Return disabled, because
it was incorrect; returns can have loops for children. Restored in
a later CL.)
Includes support for -d=loopvarhash=<binary string> intended for use
with hash search and GOCOMPILEDEBUG=loopvarhash=<binary string>
(use `gossahash -e loopvarhash command-that-fails`).
Minor feature upgrades to hash-triggered features; clients can specify
that file-position hashes use only the most-inline position, and/or that
they use only the basenames of source files (not the full directory path).
Most-inlined is the right choice for debugging loop-iteration change
once the semantics are linked to the package across inlining; basename-only
makes it tractable to write tests (which, otherwise, depend on the full
pathname of the source file and thus vary).
Updates #57969.
Change-Id: I180a51a3f8d4173f6210c861f10de23de8a1b1db
Reviewed-on: https://go-review.googlesource.com/c/go/+/411904
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This patch changes the compiler's pkg init machinery to pick out large
initialization assignments to global maps (e.g.
var mymap = map[string]int{"foo":1, "bar":2, ... }
and extract the map init code into a separate outlined function, which is
then called from the main init function with a weak relocation:
var mymap map[string]int // KEEP reloc -> map.init.0
func init() {
map.init.0() // weak relocation
}
func map.init.0() {
mymap = map[string]int{"foo":1, "bar":2}
}
The map init outlining is done selectively (only in the case where the
RHS code exceeds a size limit of 20 IR nodes).
In order to ensure that a given map.init.NNN function is included when
its corresponding map is live, we add dummy R_KEEP relocation from the
map variable to the map init function.
This first patch includes the main compiler compiler changes, and with
the weak relocation addition disabled. Subsequent patch includes the
requred linker changes along with switching to the call to the
outlined routine to a weak relocation. See the later linker change for
associated compile time performance numbers.
Updates #2559.
Updates #36021.
Updates #14840.
Change-Id: I1fd6fd6397772be1ebd3eb397caf68ae9a3147e9
Reviewed-on: https://go-review.googlesource.com/c/go/+/461315
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sym.Def used to be used for symbol resolution during the
old (pre-types2) typechecker. But since moving to types2-based IR
construction, we haven't really had a need for Sym.Def to ever refer
to anything but the package-scope definition, because types2 handles
symbol resolution for us.
This CL finally removes the Markdcl/Pushdcl/Popdcl functions that have
been a recurring source of issues in the past.
Change-Id: I2b012a0f17203efdd724ebd1e9314bd128cc2d61
Reviewed-on: https://go-review.googlesource.com/c/go/+/458625
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
This flag forced the compiler to eagerly type check all available
inline function bodies, which presumably was useful in the early days
of implementing inlining support. However, it shouldn't have any
significance with the unified frontend, since the same code paths are
used for constructing normal function bodies as for inlining.
Updates #57410.
Change-Id: I6842cf86bcd0fbf22ac336f2fc0b7b8fe14bccca
Reviewed-on: https://go-review.googlesource.com/c/go/+/458617
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL removes the GOEXPERIMENT=nounified knob, and any conditional
statements that depend on that knob. Further CLs to remove unreachable
code follow this one.
Updates #57410.
Change-Id: I39c147e1a83601c73f8316a001705778fee64a91
Reviewed-on: https://go-review.googlesource.com/c/go/+/458615
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
This was disabled in CL 452676 out of an abundance of caution,
but further analysis has shown that the failures were not being
caused by this optimization. Instead the sequence of commits was:
CL 450136 cmd/compile: handle simple inlined calls in staticinit
...
CL 449937 archive/tar, archive/zip: return ErrInsecurePath for unsafe paths
...
CL 451555 cmd/compile: fix static init for inlined calls
The failures in question became compile failures in the first CL
and started building again after the last CL.
But in the interim the code had been broken by the middle CL.
CL 451555 was just the first time that the tests could run and fail.
For #30820.
Change-Id: I65064032355b56fdb43d9731be2f9f32ef6ee600
Reviewed-on: https://go-review.googlesource.com/c/go/+/452817
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL adds -d=inlstaticinit to control whether static initialization
of inlined function calls (added in CL 450136) is allowed.
We've needed to fix it once already (CL 451555) and Google-internal
testing is hitting additional failure cases, so putting this
optimization behind a feature flag seems appropriate regardless.
Also, while we diagnose and fix the remaining cases, this CL also
disables the optimization to avoid miscompilations.
Updates #56894.
Change-Id: If52a358ad1e9d6aad1c74fac5a81ff9cfa5a3793
Reviewed-on: https://go-review.googlesource.com/c/go/+/452676
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
This CL changes cmd/compile to reject anonymous interface cycles like:
type I interface { m() interface { I } }
We don't anticipate any users to be affected by this change in
practice. Nonetheless, this CL also adds a `-d=interfacecycles`
compiler flag to suppress the error. And assuming no issue reports
from users, we'll move the check into go/types and types2 instead.
Updates #56103.
Change-Id: I1f1dce2d7aa19fb388312cc020e99cc354afddcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/445598
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
On advice of the department of garbage collection, forcing a garbage
collection generally does not improve performance. However,
this-data-is-now-unreachable is a good property to be able to test,
and that requires finalizers and a forced GC. So, to save build time,
this test was removed from the compiler itself, but to verify the
property, it was added to the fma_test (and the end-to-end dependence
on the flag was tested with an inserted failure in testing the
test).
TODO: also turn on the new -d=gccheck=1 debug flag on the ssacheck
builder.
Benchmarking reveals that it is profitable to avoid this GC,
with about 1.5% reduction in both user and wall time.
(48 p) https://perf.golang.org/search?q=upload:20221103.3
(12 p) https://perf.golang.org/search?q=upload:20221103.5
Change-Id: I4c4816d619735838a32388acf0cc5eb1cd5f0db5
Reviewed-on: https://go-review.googlesource.com/c/go/+/447359
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Adjust PGO inlining default parameters to 99% CDF threshold and
2000 budget. Benchmark results (mostly from Sweet) show that this
set of parameters performs reasonably well, with a few percent
speedup at the cost of a few percent binary size increase.
Also rename the debug flags to start with "pgo", to make it clear
that they are related to PGO.
Change-Id: I0749249b1298d1dc55a28993c37b3185f9d7639d
Reviewed-on: https://go-review.googlesource.com/c/go/+/449477
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This gets the Go command out of the business of thinking it understands
compiler debug flags, and allows the compiler to turn down its worker
concurrency instead of failing and forcing the user to do the very
same thing. Debug flags that are obviously safe for concurrency
(at least to me) are tagged; probably there's more.
Change-Id: I59bb19861d8a654a9cfd2364ee78c8628212f82e
Reviewed-on: https://go-review.googlesource.com/c/go/+/448359
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Benchmarking suggests about a 14-17% reduction in user build time,
about 3.5-7.8% reduction for wall time. This helps most builds
because small packages are common. Latest benchmarks (after the last
round of improvement):
(12 processors) https://perf.golang.org/search?q=upload:20221102.20
(GOMAXPROCS=2) https://perf.golang.org/search?q=upload:20221103.1
(48 processors) https://perf.golang.org/search?q=upload:20221102.19
(The number of compiler workers is capped at min(4, GOMAXPROCS))
An earlier, similar version of this CL at one point observed a 27%
reduction in user build time (building 40+ benchmarks, 20 times), but
the current form is judged to be the most reliable; it may be
profitable to tweak the numbers slightly later, and/or to adjust the
number of compiler workers.
We've talked about doing this in the past, the "new"(ish) metrics
package makes it a more tractable proposition.
The method here is:
1. If os.Getenv("GOGC") is empty, then increase GOGC to a large value,
calculated to grow the heap to 32 + 4 * compile_parallelism before a
GC occurs (e.g., on a >= 4 processor box, 64M). In practice,
sometimes GC occurs before that, but this still results in fewer GCs
and saved time. This is "heap goal".
2. Use a finalizer to approximately detect when GC occurs, and use
runtime metrics to track progress towards the goal heap size,
readjusting GOGC to retarget it as necessary. Reset GOGC to 100 when
the heap is "close enough" to the goal.
One feared failure mode of doing this is that the finalizer will be
slow to run and the heap will grow exceptionally large before GOGC is
reset; I monitored the heap size at reset and exit across several
boxes with a variety of processor counts and extra noise
(including several builds in parallel, including a laptop with a busy
many-tabs browser running) and overshoot effectively does not occur.
In some cases the compiler's heap grows so rapidly that estimated live
exceeds the GC goal, but this is not delayed-finalizer overshoot; the
compiler is just using that much memory. In a small number of cases
(3% of GCs in make.bash) the new goal is larger than predicted by as
much as 38%, so check for that and redo the adjustment.
I considered instead using the maximum heap size limit +
GC-detecting-finalizer + reset instead, but to me that seemed like it
might have a worse bad-case outcome; if the reset is delayed, it's
possible the GC would start running frequently, making it harder to
run the finalizer, reach 50% utilization, and the extra GCs would
lose the advantage. This might also perform badly in the case that a
rapidly growing heap outruns goal. In practice, this sort of
overshoot hasn't been observed, and a goal of 64M is small enough to
tolerate plenty of overshoot anyway.
This version of the CL includes a comment urging anyone who sees the
code and thinks it would work for them, to update a bug (to be
created if the CL is approved) with information about their
situation/experience, so that we may consider creating some more
official and reliable way of obtaining the same result.
Change-Id: I45df1c927c1a7d7503ade1abd1a3300e27516633
Reviewed-on: https://go-review.googlesource.com/c/go/+/436235
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This adds a -d debug flag "fmahash" for hashcode search for
floating point architecture-dependent problems. This variable has no
effect on architectures w/o fused-multiply-add.
This was rebased onto the GOSSAHASH renovation so that this could have
its own dedicated environment variable, and so that it would be
cheap (a nil check) to check it in the normal case.
Includes a basic test of the trigger plumbing.
Sample use (on arm64, ppc64le, s390x):
% GOCOMPILEDEBUG=fmahash=001110110 \
go build -o foo cmd/compile/internal/ssa/testdata/fma.go
fmahash triggered main.main:24 101111101101111001110110
GOFMAHASH triggered main.main:20 010111010000101110111011
1.0000000000000002 1.0000000000000004 -2.220446049250313e-16
exit status 1
The intended use is in conjunction with github.com/dr2chase/gossahash,
which will probably acquire a flag "-fma" to streamline its use. This
tool+use was inspired by an ad hoc use of this technique "in anger"
to debug this very problem. This is also a dry-run for using this
same technique to identify code sensitive to loop variable
lifetime/capture, should we make that change.
Example intended use, with current search tool (using old environment
variable), for a test example:
gossahash -e GOFMAHASH GOMAGIC=GOFMAHASH go run fma.go
Trying go args=[...], env=[GOFMAHASH=1 GOMAGIC=GOFMAHASH]
go failed (81 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=11 GOMAGIC=GOFMAHASH]
go failed (39 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=011 GOMAGIC=GOFMAHASH]
go failed (18 distinct triggers): exit status 1
Trying go args=[...], env=[GOFMAHASH=0011 GOMAGIC=GOFMAHASH]
Trying go args=[...], env=[GOFMAHASH=1011 GOMAGIC=GOFMAHASH]
...
Trying go args=[...], env=[GOFMAHASH=0110111011 GOMAGIC=GOFMAHASH]
Trying go args=[...], env=[GOFMAHASH=1110111011 GOMAGIC=GOFMAHASH]
go failed (2 distinct triggers): exit status 1
Trigger string is 'GOFMAHASH triggered math.qzero:427 111111101010011110111011', repeated 6 times
Trigger string is 'GOFMAHASH triggered main.main:20 010111010000101110111011', repeated 1 times
Trying go args=[...], env=[GOFMAHASH=01110111011 GOMAGIC=GOFMAHASH]
go failed (1 distinct triggers): exit status 1
Trigger string is 'GOFMAHASH triggered main.main:20 010111010000101110111011', repeated 1 times
Review GSHS_LAST_FAIL.0.log for failing run
FINISHED, suggest this command line for debugging:
GOSSAFUNC='main.main:20 010111010000101110111011' \
GOFMAHASH=01110111011 GOMAGIC=GOFMAHASH go run fma.go
Change-Id: Ifa22dd8f1c37c18fc8a4f7c396345a364bc367d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/394754
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Currently in PGO we use a percentage threshold to determine if a
callsite is hot. This CL uses a different method -- treating the
hottest callsites that make up cumulatively top X% of total edge
weights as hot (X=95 for now). This default might work better for
a wider range of profiles. (The absolute threshold can still be
changed by a flag.)
For #55022.
Change-Id: I7e3b6f0c3cf23f9a89dd5994c10075b498bf14ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/447016
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Currently, with PGO, the inliner uses node weights to decide if a
function is inlineable (with a larger budget). But the actual
inlining is determined by the weight of the call edge. There is a
discrepancy that, if a callee node is hot but the call edge is not,
it would not inlined, and marking the callee inlineable would of
no use.
Instead of using two kinds of weights, we just use the edge
weights to decide inlineability. If a function is the callee of a
hot call edge, its inlineability is determined with a larger
threshold. For a function that exceeds the regular inlining budget,
it is still inlined only when the call edge is hot, as it would
exceed the regular inlining cost for non-hot call sites, even if
it is marked inlineable.
For #55022.
Change-Id: I93fa9919fc6bcbb394e6cfe54ec96a96eede08f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/447015
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Randomized feature enable/disable might be something we use to
help users debug any problems with changed loop variable capture,
and there's another CL that would like to use it to help in
locating places where "fused" multiply add instructions change
program behavior.
This CL:
- adds the ability to include an integer parameter (e.g. line number)
- replumbed the environment variable into a flag to simplify go build cache management
- but added an environment variable to allow flag setting through the environment
- which adds the possibility of switching on a different variable
(if there's one built-in for variable capture, it shouldn't be GOSSAHASH)
- cleaned up the checking code
- adds tests for all the intended behavior
- removes the case for GSHS_LOGFILE; TBD whether we'll need to put that back
or if there is another way.
Change-Id: I8503e1bb3dbc4a743aea696e04411ea7ab884787
Reviewed-on: https://go-review.googlesource.com/c/go/+/443063
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Also removes no-longer-needed "Any" field from compiler's DebugFlags.
Test/use case for this is the fmahash CL.
Change-Id: I214f02c91f30fc2ce53caf75fa5e2b905dd33429
Reviewed-on: https://go-review.googlesource.com/c/go/+/445495
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Shape-based stenciling in unified IR is done by converting type argument
to its underlying type. So it agressively check that type argument is
not a TFORW. However, for recursive instantiated type argument, it may
still be a TFORW when shapifying happens. Thus the assertion failed,
causing the compiler crashing.
To fix it, just allow fully instantiated type when shapifying.
Fixes#54512Fixes#54722
Change-Id: I527e3fd696388c8a37454e738f0324f0c2ec16cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/426335
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
In CL 424734, I implemented pointer shaping for unified IR. Evidently
though, we didn't have any test cases that check that uses of
pointer-shaped expressions were handled correctly.
In the reported test case, the struct field "children items[*node[T]]"
gets shaped to "children items[go.shape.*uint8]" (underlying type
"[]go.shape.*uint8"); and so the expression "n.children[i]" has type
"go.shape.*uint8" and the ".items" field selection expression fails.
The fix implemented in this CL is that any expression of derived type
now gets an explicit "reshape" operation applied to it, to ensure it
has the appropriate type for its context. E.g., the "n.children[i]"
OINDEX expression above gets "reshaped" from "go.shape.*uint8" to
"*node[go.shape.int]", allowing the field selection to succeed.
This CL also adds a "-d=reshape" compiler debugging flag, because I
anticipate debugging reshaping operations will be something to come up
again in the future.
Fixes#54535.
Change-Id: Id847bd8f51300d2491d679505ee4d2e974ca972a
Reviewed-on: https://go-review.googlesource.com/c/go/+/424936
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: hopehook <hopehook@qq.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The Go object file references (some of) symbols from other
packages by indices, not by names. The linker doesn't need the
symbol names to do the linking. The names are included in the
object file so it is self-contained and tools (objdump, nm) can
read the referenced symbol names. Including the names increases
object file size. Add a flag to disable it on demand (off by
default).
Change-Id: I143a0eb656997497c750b8eb1541341b2aee8f30
Reviewed-on: https://go-review.googlesource.com/c/go/+/404297
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Unified IR quirks mode existed to help bootstrap unified IR by forcing
it to produce bit-for-bit identical output to the original gc noder
and typechecker. However, I believe it's far enough along now to stand
on its own, plus we have good test coverage of generics already for
-G=3 mode.
Change-Id: I8bf412c8bb5d720eadeac3fe31f49dc73679da70
Reviewed-on: https://go-review.googlesource.com/c/go/+/385998
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This adds a debugging hook for optionally calling a "maymorestack"
function in the prologue of any function that might call morestack
(whether it does at run time or not). The maymorestack function will
let us improve lock checking and add debugging modes that stress
function preemption and stack growth.
Passes toolstash-check -all (except on js/wasm, where toolstash
appears to be broken)
Fixes#48297.
Change-Id: I27197947482b329af75dafb9971fc0d3a52eaf31
Reviewed-on: https://go-review.googlesource.com/c/go/+/359795
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
This moves and slightly generalizes the -d debug flag parser from
cmd/compile/internal/base to cmd/internal/objabi so that we can use
the same debug flag syntax in other tools.
This makes a few minor tweaks to implementation details. The flag
itself is now just a flag.Value that gets constructed explicitly,
rather than at init time, and we've cleaned up the implementation a
little (e.g., using a map instead of a linear search of a slice). The
help text is now automatically alphabetized. Rather than describing
the values of some flags in the help text footer, we simply include it
in the flags' help text and make sure multi-line help text renders
sensibly.
For #48297.
Change-Id: Id373ee3b767e456be483fb28c110d025149be532
Reviewed-on: https://go-review.googlesource.com/c/go/+/359956
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Originally, overloading -d=inlfuncswithclosures=0 to enable quirks
mode was convenient because toolstash -cmp doesn't provide a way to
pass different gcflags to the installed vs stashed toolchains. Prior
to unified IR being merged, the stashed toolchain wouldn't know about
or accept any unified-specific flags.
However, this concern is no longer applicable since unified IR has
been merged, and the TestUnifiedCompare test can easily specify
different flag sets for the baseline and experiment build configs.
This CL adds a new -d=unifiedquirks flag to enable quirks mode, so
that it's possible to test unified IR with -d=inlfuncswithclosures=0
without also affecting a bunch of other compilation details.
Change-Id: Id1932f332822622aa8617278e82ec6d1a53b1b46
Reviewed-on: https://go-review.googlesource.com/c/go/+/329733
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
This CL extends the unified export data format's existing sync
mechanism to save writer stacks, controlled by the -d=syncframes debug
flag. This allows readers to provide more details when reporting
desync errors, which should simplify development of the data format
and the various reader/writer implementations.
For example, CL 328051 updated reader and writer, but missed making a
similar change to the linker (fix in CL 328054). Re-reviewing the CL
in isolation after the failure, it was not immediately obvious what
was going wrong. But the pair of stack traces below identifies exactly
what happened: it should have updated linker.relocFuncExt to write out
the new sync marker too.
```
data sync error: package "internal/abi", section 6, index 4, offset 536
found UseReloc, written at:
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/encoder.go:221: (*encoder).reloc +0x44
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:214: (*linker).relocFuncExt +0x580
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:233: (*linker).relocTypeExt +0x234
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:161: (*linker).relocObj +0x2198
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:64: (*linker).relocIdx +0x196
expected ImplicitTypes, reading at:
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:796: (*reader).implicitTypes +0x36
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:810: (*reader).addBody +0x81
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:727: (*reader).funcExt +0x542
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:651: (*reader).method +0x324
/home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:557: (*pkgReader).objIdx +0x2704
```
Change-Id: I911193edd2a965f81b7459f15fb613a773584685
Reviewed-on: https://go-review.googlesource.com/c/go/+/328909
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
This CL adds a new -d=unified debug flag, which controls whether
unified IR mode is used.
Change-Id: Iaa5f3cc0a24b9881aeec5317cd6b462b4a7b6fc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/327054
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
This moves all remaining GOEXPERIMENT flags into the objabi.Experiment
struct, drops the "_enabled" from their name, and makes them all bool
typed.
We also drop DebugFlags.Fieldtrack because the previous CL shifted the
one test that used it to use GOEXPERIMENT instead.
Change-Id: I3406fe62b1c300bb4caeaffa6ca5ce56a70497fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/302389
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Added a flag '-d=inlfuncswithclosures=1' to allow inlining functions with
closures, and change the default to off for now, until #44370 is fixed.
Updates #44370.
Change-Id: Ic17723aa5c091d91f5f5004d8b63ec7125257acf
Reviewed-on: https://go-review.googlesource.com/c/go/+/296049
Run-TryBot: Dan Scales <danscales@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dan Scales <danscales@google.com>
The compiler currently has two modes for compilation: one where it
compiles each function as it sees them, and another where it enqueues
them all into a work queue. A subsequent CL is going to reorder
function compilation to ensure that functions are always compiled
before any non-trivial function literals they enclose, and this will
be easier if we always use the compile work queue.
Also, fewer compilation modes makes things simpler to reason about.
Change-Id: Ie090e81f7476c49486296f2b90911fa0a466a5dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/283313
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Add compiler support for emitting ABI wrappers by creating real IR as
opposed to introducing ABI aliases. At the moment these are "no-op"
wrappers in the sense that they make a simple call (using the existing
ABI) to their target. The assumption here is that once late call
expansion can handle both ABI0 and the "new" ABIInternal (register
version), it can expand the call to do the right thing.
Note that the runtime contains functions that do not strictly follow
the rules of the current Go ABI0; this has been handled in most cases
by treating these as ABIInternal instead (these changes have been made
in previous patches).
Generation of ABI wrappers (as opposed to ABI aliases) is currently
gated by GOEXPERIMENT=regabi -- wrapper generation is on by default if
GOEXPERIMENT=regabi is set and off otherwise (but can be turned on
using "-gcflags=all=-abiwrap -ldflags=-abiwrap"). Wrapper generation
currently only workd on AMD64; explicitly enabling wrapper for other
architectures (via the command line) is not supported.
Also in this patch are a few other command line options for debugging
(tracing and/or limiting wrapper creation). These will presumably go
away at some point.
Updates #27539, #40724.
Change-Id: I1ee3226fc15a3c32ca2087b8ef8e41dbe6df4a75
Reviewed-on: https://go-review.googlesource.com/c/go/+/270863
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Than McIntosh <thanm@google.com>
Move Flag, Debug, Ctxt, Exit, and error messages to
new package cmd/compile/internal/base.
These are the core functionality that everything in gc uses
and which otherwise prevent splitting any other code
out of gc into different packages.
A minor milestone: the compiler source code
no longer contains the string "yy".
[git-generate]
cd src/cmd/compile/internal/gc
rf '
mv atExit AtExit
mv Ctxt atExitFuncs AtExit Exit base.go
mv lineno Pos
mv linestr FmtPos
mv flusherrors FlushErrors
mv yyerror Errorf
mv yyerrorl ErrorfAt
mv yyerrorv ErrorfVers
mv noder.yyerrorpos noder.errorAt
mv Warnl WarnfAt
mv errorexit ErrorExit
mv base.go debug.go flag.go print.go cmd/compile/internal/base
'
: # update comments
sed -i '' 's/yyerrorl/ErrorfAt/g; s/yyerror/Errorf/g' *.go
: # bootstrap.go is not built by default so invisible to rf
sed -i '' 's/Fatalf/base.Fatalf/' bootstrap.go
goimports -w bootstrap.go
: # update cmd/dist to add internal/base
cd ../../../dist
sed -i '' '/internal.amd64/a\
"cmd/compile/internal/base",
' buildtool.go
gofmt -w buildtool.go
Change-Id: I59903c7084222d6eaee38823fd222159ba24a31a
Reviewed-on: https://go-review.googlesource.com/c/go/+/272250
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
