Dan Scales [Tue, 1 Dec 2020 22:48:03 +0000 (14:48 -0800)]
[dev.regabi] cmd/compile: exporting, importing, and inlining functions with OCLOSURE
I have exporting, importing, and inlining of functions with closures
working in all cases (issue #28727). all.bash runs successfully without
errors.
Approach:
- Write out the Func type, Dcls, ClosureVars, and Body when exporting
an OCLOSURE.
- When importing an OCLOSURE, read in the type, dcls, closure vars,
and body, and then do roughly equivalent code to (*noder).funcLit
- During inlining of a closure within inlined function, create new
nodes for all params and local variables (including closure
variables), so they can have a new Curfn and some other field
values. Must substitute not only on the Nbody of the closure, but
also the Type, Cvars, and Dcl fields.
Fixes #28727
Change-Id: I4da1e2567c3fa31a5121afbe82dc4e5ee32b3170
Reviewed-on: https://go-review.googlesource.com/c/go/+/283112
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
Matthew Dempsky [Sun, 17 Jan 2021 10:53:18 +0000 (02:53 -0800)]
[dev.regabi] cmd/compile: directly create go.map and go.track symbols
These symbols are implementation details and don't correspond to Go
source symbols, so directly create them as linker symbols and get rid
of their pseudo packages.
Passes toolstash -cmp w/ -gcflags=all=-abiwrap.
Change-Id: I2e97374c21f3e909f6d350f15e7a5ed3574cadf4
Reviewed-on: https://go-review.googlesource.com/c/go/+/284372
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Currently there's a lot of logic within package types for creating
Linksyms. This CL pulls it out into base, where it can be more easily
reused by other compiler code that shouldn't need to depend on package
types.
Package base probably isn't the best place for this, but it's
convenient because it's a package that types already depends on. It's
also where the Ctxt object lives, which these functions depend upon.
Passes toolstash -cmp w/ -gcflags=all=-abiwrap.
Change-Id: I50d8b7e4596955205036969eab24d7dab053b363
Reviewed-on: https://go-review.googlesource.com/c/go/+/284231
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Sun, 17 Jan 2021 08:46:42 +0000 (00:46 -0800)]
[dev.regabi] cmd/compile: cleanup callTargetLSym
Now that TailCallStmt carries an *ir.Name instead of a *types.Sym,
callTargetLSym can be similarly updated to take the target function as
an *ir.Name.
This inches us closer towards being able to move Linksym and other
properties from *types.Sym to *ir.Name, where they belong.
Passes toolstash -cmp w/ -gcflags=all=-abiwrap.
Change-Id: I091da290751970eba8ed0438f66d6cca88b665a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/284228
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
Matthew Dempsky [Mon, 18 Jan 2021 06:05:50 +0000 (22:05 -0800)]
[dev.regabi] cmd/compile: move stack objects to liveness
Calculating and emitting stack objects are essentially part of
liveness analysis, so move the code from ssagen to liveness. Allows
unexporting liveness.ShouldTrack.
Passes toolstash -cmp.
Change-Id: I88b5b2e75b8dfb46b8b03a2fa09a9236865cbf3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/284413
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Matthew Dempsky [Mon, 18 Jan 2021 00:14:48 +0000 (16:14 -0800)]
[dev.regabi] cmd/compile: convert OPANIC argument to interface{} during typecheck
Currently, typecheck leaves arguments to OPANIC as their original
type. This CL changes it to insert implicit OCONVIFACE operations to
convert arguments to `interface{}` like how any other function call
would be handled.
No immediate benefits, other than getting to remove a tiny bit of
special-case logic in order.go's handling of OPANICs. Instead, the
generic code path for handling OCONVIFACE is used, if necessary.
Longer term, this should be marginally helpful for #43753, as it
reduces the number of cases where we need values to be addressable for
runtime calls.
However, this does require adding some hacks to appease existing
tests:
1. We need yet another kludge in inline budgeting, to ensure that
reflect.flag.mustBe stays inlinable for cmd/compile/internal/test's
TestIntendedInlining.
2. Since the OCONVIFACE expressions are now being introduced during
typecheck, they're now visible to escape analysis. So expressions like
"panic(1)" are now seen as "panic(interface{}(1))", and escape
analysis warns that the "interface{}(1)" escapes to the heap. These
have always escaped to heap, just now we're accurately reporting about
it.
(Also, unfortunately fmt.go hides implicit conversions by default in
diagnostics messages, so instead of reporting "interface{}(1) escapes
to heap", it actually reports "1 escapes to heap", which is
confusing. However, this confusing messaging also isn't new.)
Change-Id: Icedf60e1d2e464e219441b8d1233a313770272af
Reviewed-on: https://go-review.googlesource.com/c/go/+/284412
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Cuong Manh Le [Sun, 17 Jan 2021 09:41:19 +0000 (16:41 +0700)]
[dev.regabi] cmd/compile: use *obj.LSym instead of *ir.Name for staticdata functions
Those functions only use (*ir.Name).Linksym(), so just change them to
get an *obj.LSym directly. This helps get rid of un-necessary
validations that their callers have already done.
Passes toolstash -cmp.
For #43737.
Change-Id: Ifd6c2525e472f8e790940bc167665f9d74dd1bc5
Reviewed-on: https://go-review.googlesource.com/c/go/+/284121
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Sun, 17 Jan 2021 10:16:58 +0000 (02:16 -0800)]
[dev.regabi] cmd/compile: remove unneeded packages from ir.Pkgs
ir.Pkgs.Itablink isn't used anymore. (I don't recall what it was ever
used for.)
ir.Pkgs.Race and ir.Pkgs.Msan are only needed in exactly only place,
so just create them on demand there, the same way that we create
"main" on demand.
Change-Id: I3474bb949f71cd40c7a462b9f4a369adeacde0d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/284230
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 17 Jan 2021 08:30:32 +0000 (00:30 -0800)]
[dev.regabi] cmd/compile: add ir.TailCallStmt
This CL splits out ORETJMP as a new TailCallStmt node, separate from
the other BranchStmt nodes. In doing so, this allows us to change it
from identifying a function by *types.Sym to identifying one by
directly pointing to the *ir.Func.
While here, also rename the operation to OTAILCALL.
Passes toolstash -cmp.
Change-Id: I273e6ea5d92bf3005ae02fb59b3240a190a6cf1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/284227
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
CL 284223 tightened down the allowed expressions in mayCall, but
evidently a little too tight. The linux-amd64-noopt builder does in
fact see expressions with non-empty Init lists in arguments list.
Since I believe these can only appear on the RHS of LogicalExpr
expressions, this CL relaxes that one case.
Change-Id: I1e6bbd0449778c40ed2610b3e1ef6a825a84ada7
Reviewed-on: https://go-review.googlesource.com/c/go/+/284226
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This CL simplifies the previous one a little bit further, by combining
reordering stack-temporary initialization and getting rid of an
unneeded temporary variable. (Does not pass toolstash -cmp.)
Change-Id: I17799dfe368484f33a8ddd0ab4f68647d6262147
Reviewed-on: https://go-review.googlesource.com/c/go/+/284225
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 17 Jan 2021 00:59:19 +0000 (16:59 -0800)]
[dev.regabi] cmd/compile: replace Node.HasCall with walk.mayCall
After CL 284220, we now only need to detect expressions that contain
function calls in the arguments list of further function calls. So we
can simplify Node.HasCall/fncall/etc a lot.
Instead of incrementally tracking whether an expression contains
function calls all throughout walk, simply check once at the point of
using an expression as a function call argument. Since any expression
checked here will itself become a function call argument, it won't be
checked again because we'll short circuit at the enclosing function
call.
Also, restructure the recursive walk code to use mayCall, and trim
down the list of acceptable expressions. It should be okay to be
stricter, since we'll now only see function call arguments and after
they've already been walked.
It's possible I was overly aggressive removing Ops here. But if so,
we'll get an ICE, and it'll be easy to re-add them. I think this is
better than the alternative of accidentally allowing expressions
through that risk silently clobbering the stack.
Passes toolstash -cmp.
Change-Id: I585ef35dcccd9f4018e4bf2c3f9ccb1514a826f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/284223
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Currently, to ensure OAS2FUNC results are assigned in the correct
order, they're always assigned to temporary variables. However, these
temporary variables are typed based on the destination type, which may
require an interface conversion. This means walk may have to then
introduce a second set of temporaries to ensure result parameters are
all copied out of the results area, before it emits calls to runtime
conversion functions.
That's just silly. Instead, this CL changes order to allocate the
result temporaries with the same type as the function returns in the
first place, and then assign them one at a time to their destinations,
with conversions as needed.
While here, also fix an order-of-evaluation issue with has-ok
assignments that I almost added to multi-value function call
assignments, and add tests for each.
Change-Id: I9f4e962425fe3c5e3305adbbfeae2c7f253ec365
Reviewed-on: https://go-review.googlesource.com/c/go/+/284220
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Dan Scales [Fri, 15 Jan 2021 22:12:35 +0000 (14:12 -0800)]
[dev.regabi] cmd/compile, runtime: fix up comments/error messages from recent renames
Went in a semi-automated way through the clearest renames of functions,
and updated comments and error messages where it made sense.
Change-Id: Ied8e152b562b705da7f52f715991a77dab60da35
Reviewed-on: https://go-review.googlesource.com/c/go/+/284216
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
CL 283233 added reflectType method to ssagen.state, which we can use to
setup type address in the SSA backend in favor of the frontend. However,
this will change the order of symbols generation, so not safe for toolstash.
Change-Id: Ib6932ec42a9d28c3fd7a1c055596e75494c29843
Reviewed-on: https://go-review.googlesource.com/c/go/+/284115
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
WriteType isn't safe for direct concurrent use, and users should
instead use TypeLinksym or another higher-level API provided by
reflectdata. After the previous CL, there are no remaining uses of
WriteType elsewhere in the compiler, so unexport it to keep it that
way.
For #43701.
[git-generate]
cd src/cmd/compile/internal/reflectdata
rf '
mv WriteType writeType
'
Change-Id: I294a78be570a47feb38a1ad4eaae7723653d5991
Reviewed-on: https://go-review.googlesource.com/c/go/+/284077
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Fri, 15 Jan 2021 08:56:02 +0000 (00:56 -0800)]
[dev.regabi] cmd/compile: move stkobj symbol generation to SSA
The code for allocating linksyms and recording that we need runtime
type descriptors is now concurrent-safe, so move it to where those
symbols are actually needed to reduce complexity and risk of failing
to generate all needed symbols in advance.
For #43701.
Change-Id: I759d2508213ac9a4e0b504b51a75fa10dfa37a8d
Reviewed-on: https://go-review.googlesource.com/c/go/+/284076
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Fri, 15 Jan 2021 08:39:24 +0000 (00:39 -0800)]
[dev.regabi] cmd/compile: don't promote Byval CaptureVars if Addrtaken
We decide during escape analysis whether to pass closure variables by
value or reference. One of the factors that's considered is whether a
variable has had its address taken.
However, this analysis is based only on the user-written source code,
whereas order+walk may introduce rewrites that take the address of a
variable (e.g., passing a uint16 key by reference to the size-generic
map runtime builtins).
Typically this would be harmless, albeit suboptimal. But in #43701 it
manifested as needing a stack object for a function where we didn't
realize we needed one up front when we generate symbols.
Probably we should just generate symbols on demand, now that those
routines are all concurrent-safe, but this is a first fix.
Thanks to Alberto Donizetti for reporting the issue, and Cuong Manh Le
for initial investigation.
Fixes #43701.
Change-Id: I16d87e9150723dcb16de7b43f2a8f3cd807a9437
Reviewed-on: https://go-review.googlesource.com/c/go/+/284075
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
David Chase [Fri, 15 Jan 2021 01:10:35 +0000 (20:10 -0500)]
[dev.regabi] test: disable test on windows because expected contains path separators.
The feature being tested is insensitive to the OS anyway.
Change-Id: Ieac9bfaafc6a54c00017afcc0b87bd8bbe80af7b
Reviewed-on: https://go-review.googlesource.com/c/go/+/284032
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
Matthew Dempsky [Fri, 15 Jan 2021 03:40:07 +0000 (19:40 -0800)]
[dev.regabi] cmd/compile: fix ICE during ir.Dump
fmt.go:dumpNodeHeader uses reflection to call all "func() bool"-typed
methods on Nodes during printing, but the OnStack method that I added
in CL 283233 isn't meant to be called on non-variables.
dumpNodeHeader does already guard against panics, as happen in some
other accessors, but not against Fatalf, as I was using in OnStack. So
simply change OnStack to use panic too.
Thanks to drchase@ for the report.
Change-Id: I0cfac84a96292193401a32fc5e7fd3c48773e008
Reviewed-on: https://go-review.googlesource.com/c/go/+/284074
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Than McIntosh [Tue, 22 Dec 2020 21:48:13 +0000 (16:48 -0500)]
[dev.regabi] cmd/compile,cmd/link: additional code review suggestions for CL 270863
This patch pulls in a few additional changes requested by code
reviewers for CL 270863 that were accidentally left out. Specifically,
guarding use of ORETJMP to insure it is not used when building dynlink
on ppc64le, and a tweaking the command line flags used to control
wrapper generation.
Change-Id: I4f96462e570180887eb8693e11badd83d142710a
Reviewed-on: https://go-review.googlesource.com/c/go/+/279527
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Than McIntosh <thanm@google.com>
Matthew Dempsky [Tue, 12 Jan 2021 06:58:23 +0000 (22:58 -0800)]
[dev.regabi] cmd/compile: move more PAUTOHEAP to SSA construction
This CL moves almost all PAUTOHEAP handling code to SSA construction.
Instead of changing Names to PAUTOHEAP, escape analysis now only sets
n.Esc() to ir.EscHeap, and SSA handles creating the "&x"
pseudo-variables and associating them via Heapaddr.
This CL also gets rid of n.Stackcopy, which was used to distinguish
the heap copy of a parameter used within a function from the stack
copy used in the function calling convention. In practice, this is
always obvious from context: liveness and function prologue/epilogue
want to know about the stack copies, and everywhere else wants the
heap copy.
Hopefully moving all parameter/result handling into SSA helps with
making the register ABI stuff easier.
Also, the only remaining uses of PAUTOHEAP are now for closure
variables, so I intend to rename it to PCLOSUREVAR or get rid of those
altogether too. But this CL is already big and scary enough.
Change-Id: Ief5ef6205041b9d0ee445314310c0c5a98187e77
Reviewed-on: https://go-review.googlesource.com/c/go/+/283233
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com> Reviewed-by: David Chase <drchase@google.com>
Cuong Manh Le [Wed, 6 Jan 2021 03:47:35 +0000 (10:47 +0700)]
[dev.regabi] cmd/compile: do not rely on CallExpr.Rargs for detect already walked calls
Currently, there's an awkward issue with walk pass. When walking the AST
tree, the compiler generate code for runtime functions (using mkcall* variants),
add/modify the AST tree and walk new generated tree again. This causes the
double walking on some CallExpr, which is relying on checking Rargs to prevent
that. But checking Rargs has its own issue as well.
For functions that does not have arguments, this check is failed, and we
still double walk the CallExpr node.
This CL change the way that compiler detects double walking, by using
separated field instead of relying on Rargs. In perfect world, we should make
the compiler walks the AST tree just once, but it's not safe to do that at
this moment.
Passes toolstash -cmp.
Change-Id: Ifdd1e0f98940ddb1f574af2da2ac7f005b5fcadd
Reviewed-on: https://go-review.googlesource.com/c/go/+/283672
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Wed, 13 Jan 2021 23:02:16 +0000 (15:02 -0800)]
[dev.regabi] cmd/compile: fix ICE when initializing blank vars
CL 278914 introduced NameOffsetExpr to avoid copying ONAME nodes and
hacking up their offsets, but evidently staticinit subtly depended on
the prior behavior to allow dynamic initialization of blank variables.
This CL refactors the code somewhat to avoid using NameOffsetExpr with
blank variables, and to instead create dynamic assignments directly to
the global blank node. It also adds a check to NewNameOffsetExpr to
guard against misuse like this, since I suspect there could be other
cases still lurking within staticinit. (This code is overdue for an
makeover anyway.)
Thanks to thanm@ for bisect and test case minimization.
Fixes #43677.
Change-Id: Ic71cb5d6698382feb9548dc3bb9fd606b207a172
Reviewed-on: https://go-review.googlesource.com/c/go/+/283537
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
I misread the FIXME comment in InitLSym the first time. It's referring
to how InitLSym is supposed to be called exactly once per
function (see function documentation), but this is evidently not
actually the case currently in GOEXPERIMENT=regabi mode.
So just move the NeedFuncSym call below the GOEXPERIMENT=regabi
workaround.
Also, to fix the linux-arm64-{aws,packet} builders, move the call to
reflectdata.WriteFuncSyms() to after the second batch of functions are
compiled. This is necessary to make sure we catch all the funcsyms
that can be added by late function compilation.
Change-Id: I6d6396d48e2ee29c1fb007fa2b99e065b36375db
Reviewed-on: https://go-review.googlesource.com/c/go/+/283552
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Than McIntosh <thanm@google.com>
David Chase [Fri, 8 Jan 2021 15:15:36 +0000 (10:15 -0500)]
[dev.regabi] cmd/compile: refactor abiutils from "gc" into new "abi"
Needs to be visible to ssagen, and might as well start clean to avoid
creating a lot of accidental dependencies.
Added some methods for export.
Decided to use a pointer instead of value for ABIConfig uses.
Tests ended up separate from abiutil itself; otherwise there are import cycles.
Change-Id: I5570e1e6a463e303c5e2dc84e8dd4125e7c1adcc
Reviewed-on: https://go-review.googlesource.com/c/go/+/282614
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Jeremy Faller <jeremy@golang.org>
David Chase [Mon, 4 Jan 2021 18:32:10 +0000 (13:32 -0500)]
[dev.regabi] cmd/compile: added limited //go:registerparams pragma for new ABI dev
This only works for functions; if you try it with a method, it will
fail. It does work for both local package and imports. For now,
it tells you when it thinks it sees either a declaration or a call of
such a function (this will normally be silent since no existing
code uses this pragma).
Note: it appears to be really darn hard to figure out if this
pragma was set for a method, and the method's call site. Better
ir.Node wranglers than I might be able to make headway, but it
seemed unnecessary for this experiment.
Change-Id: I601c2ddd124457bf6d62f714d7ac871705743c0a
Reviewed-on: https://go-review.googlesource.com/c/go/+/279521
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org>
David Chase [Mon, 28 Sep 2020 21:42:30 +0000 (17:42 -0400)]
[dev.regabi] cmd/compile: add code to support register ABI spills around morestack calls
This is a selected copy from the register ABI experiment CL, focused
on the files and data structures that handle spilling around morestack.
Unnecessary code from the experiment was removed, other code was adapted.
Would it make sense to leave comments in the experiment as pieces are
brought over?
Change-Id: I92136f070351d4fcca1407b52ecf9b80898fed95
Reviewed-on: https://go-review.googlesource.com/c/go/+/279520
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org>
David Chase [Mon, 4 Jan 2021 19:05:17 +0000 (14:05 -0500)]
[dev.regabi] test: make run.go error messages slightly more informative
This is intended to make it easier to write/change a test
without referring to the source code to figure out what the
error messages actually mean, or how to correct them.
Change-Id: Ie79ff7cd9f2d1fa605257fe97eace68adc8a6716
Reviewed-on: https://go-review.googlesource.com/c/go/+/281452
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jeremy Faller <jeremy@golang.org>
David Chase [Wed, 30 Dec 2020 17:05:57 +0000 (12:05 -0500)]
[dev.regabi] cmd/compile: make ordering for InvertFlags more stable
Current many architectures use a rule along the lines of
// Canonicalize the order of arguments to comparisons - helps with CSE.
((CMP|CMPW) x y) && x.ID > y.ID => (InvertFlags ((CMP|CMPW) y x))
to normalize comparisons as much as possible for CSE. Replace the
ID comparison with something less variable across compiler changes.
This helps avoid spurious failures in some of the codegen-comparison
tests (though the current choice of comparison is sensitive to Op
ordering).
Two tests changed to accommodate modified instruction choice.
Change-Id: Ib35f450bd2bae9d4f9f7838ceaf7ec682bcf1e1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/280155
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
Matthew Dempsky [Tue, 12 Jan 2021 20:25:33 +0000 (12:25 -0800)]
[dev.regabi] cmd/compile: remove Func.ClosureType
The closure's type always matches the corresponding function's type,
so just use one instance rather than carrying around two. Simplifies
construction of closures, rewriting them during walk, and shrinks
memory usage.
Passes toolstash -cmp.
Change-Id: I83b8b8f435b02ab25a30fb7aa15d5ec7ad97189d
Reviewed-on: https://go-review.googlesource.com/c/go/+/283152
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>
Matthew Dempsky [Tue, 12 Jan 2021 20:12:27 +0000 (12:12 -0800)]
[dev.regabi] cmd/compile: transform closures during walk
We used to transform directly called closures in a separate pass
before walk, because we couldn't guarantee whether we'd see the
closure call or the closure itself first. As of the last CL, this
ordering is always guaranteed, so we can rewrite calls and the closure
at the same time.
Change-Id: Ia6f4d504c24795e41500108589b53395d301123b
Reviewed-on: https://go-review.googlesource.com/c/go/+/283315
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>
Matthew Dempsky [Tue, 12 Jan 2021 20:00:58 +0000 (12:00 -0800)]
[dev.regabi] cmd/compile: compile functions before closures
This CL reorders function compilation to ensure that functions are
always compiled before any enclosed function literals. The primary
goal of this is to reduce the risk of race conditions that arise due
to compilation of function literals needing to inspect data from their
closure variables. However, a pleasant side effect is that it allows
skipping the redundant, separate compilation of function literals that
were inlined into their enclosing function.
Change-Id: I03ee96212988cb578c2452162b7e99cc5e92918f
Reviewed-on: https://go-review.googlesource.com/c/go/+/282892
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
No real code changes. Just splitting into a separate CL so the next
one is easier to review.
Change-Id: I428dc986b76370d8d3afc12cf19585f6384389d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/283314
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>
Matthew Dempsky [Tue, 12 Jan 2021 19:38:32 +0000 (11:38 -0800)]
[dev.regabi] cmd/compile: always use the compile queue
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>
Matthew Dempsky [Tue, 12 Jan 2021 19:34:00 +0000 (11:34 -0800)]
[dev.regabi] cmd/compile: call NeedFuncSym in InitLSym
InitLSym is where we're now generating ABI wrappers, so it seems as
good a place as any to make sure we're generating the degenerate
closure wrappers for declared functions and methods.
Change-Id: I097f34bbcee65dee87a97f9ed6f3f38e4cf2e2b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/283312
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>
Matthew Dempsky [Tue, 12 Jan 2021 01:22:20 +0000 (17:22 -0800)]
[dev.regabi] cmd/compile: remove Name.Typegen
Just directly set Type.Vargen when declaring defined types within a
function.
Change-Id: Idcc0007084a660ce1c39da4a3697e158a1c615b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/283212
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Matthew Dempsky [Mon, 11 Jan 2021 23:07:09 +0000 (15:07 -0800)]
[dev.regabi] cmd/compile: stop using Vargen for import/export
Historically, inline function bodies were exported as plain Go source
code, and symbol mangling was a convenient hack because it allowed
variables to be re-imported with largely the same names as they were
originally exported as.
However, nowadays we use a binary format that's more easily extended,
so we can simply serialize all of a function's declared objects up
front, and then refer to them by index later on. This also allows us
to easily report unmangled names all the time (e.g., error message
from issue7921.go).
Fixes #43633.
Change-Id: I46c88f5a47cb921f70ab140976ba9ddce38df216
Reviewed-on: https://go-review.googlesource.com/c/go/+/283193
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Trust: Dan Scales <danscales@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Mon, 11 Jan 2021 23:58:19 +0000 (15:58 -0800)]
[dev.regabi] cmd/compile: refactor closure var setup/teardown
Creating closure vars is subtle and is also needed in both CL 281932
and CL 283112, so refactor out a common implementation that can be
used in all 3 places.
Passes toolstash -cmp.
Change-Id: Ib993eb90c895b52759bfbfbaad88921e391b0b4d
Reviewed-on: https://go-review.googlesource.com/c/go/+/283194
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Trust: Dan Scales <danscales@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Matthew Dempsky [Mon, 11 Jan 2021 22:30:16 +0000 (14:30 -0800)]
[dev.regabi] cmd/compile: decouple escape analysis from Name.Vargen
Escape analysis needs to know the index of result parameters for
recording escape-flow information. It currently relies on Vargen for
this, but it can easily figure this out for itself. So just do that
instead, so that we can remove Vargen.
Passes toolstash -cmp.
For #43633.
Change-Id: I65dedc2d73bc25e85ff400f308e50b73dc503630
Reviewed-on: https://go-review.googlesource.com/c/go/+/283192
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Matthew Dempsky [Mon, 4 Jan 2021 10:24:48 +0000 (02:24 -0800)]
[dev.regabi] cmd/compile: use ClosureVars for method value wrappers
Similar to with regular closures, we can change method value wrappers
to use ClosureVars and allow SSA construction to take care of wiring
it up appropriately.
Change-Id: I05c0b1bcec4e24305324755df35b7bc5b8a6ce7a
Reviewed-on: https://go-review.googlesource.com/c/go/+/281353
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>
Matthew Dempsky [Mon, 4 Jan 2021 05:34:03 +0000 (21:34 -0800)]
[dev.regabi] cmd/compile: bind closure vars during SSA constructions
For function literals that aren't inlined or directly called, we need
to pass their arguments via a closure struct. This also means we need
to rewrite uses of closure variables to access from this closure
struct.
Currently we do this rewrite in a pass before walking begins. This CL
moves the code to SSA construction instead, alongside binding other
input parameters.
Change-Id: I13538ef3394e2d6f75d5b7b2d0adbb00db812dc2
Reviewed-on: https://go-review.googlesource.com/c/go/+/281352
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>
Matthew Dempsky [Mon, 4 Jan 2021 03:49:49 +0000 (19:49 -0800)]
[dev.regabi] cmd/compile: deref PAUTOHEAPs during SSA construction
Currently, during walk we rewrite PAUTOHEAP uses into derefs of their
corresponding Heapaddr, but we can easily do this instead during SSA
construction. This does involve updating two test cases:
* nilptr3.go
This file had a test that we emit a "removed nil check" diagnostic for
the implicit dereference from accessing a PAUTOHEAP variable. This CL
removes this diagnostic, since it's not really useful to end users:
from the user's point of view, there's no pointer anyway, so they
needn't care about whether we check for nil or not. That's a purely
internal detail. And with the PAUTOHEAP dereference handled during SSA
construction, we can more robustly ensure this happens, rather than
relying on setting a flag in walk and hoping that SSA sees it.
* issue20780.go
Previously, when PAUTOHEAPs were dereferenced during walk, it had a
consequence that when they're passed as a function call argument, they
would first get copied to the stack before being copied to their
actual destination. Moving the dereferencing to SSA had a side-effect
of eliminating this unnecessary temporary, and copying directly to the
destination parameter.
The test is updated to instead call "g(h(), h())" where h() returns a
large value, as the first result will always need to be spilled
somewhere will calling the second function. Maybe eventually we're
smart enough to realize it can be spilled to the heap, but we don't do
that today.
Because I'm concerned that the direct copy-to-parameter optimization
could interfere with race-detector instrumentation (e.g., maybe the
copies were previously necessary to ensure they're not clobbered by
inserted raceread calls?), I've also added issue20780b.go to exercise
this in a few different ways.
Change-Id: I720598cb32b17518bc10a03e555620c0f25fd28d
Reviewed-on: https://go-review.googlesource.com/c/go/+/281293
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> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Robert Griesemer [Fri, 8 Jan 2021 22:04:50 +0000 (14:04 -0800)]
[dev.regabi] cmd/compile: remove fmt_test code; it has outlived its usefulness
With the recent compiler rewrites and cleanups to gc/fmt.go, the
"safety net" provided by fmt_test has become less important and
the test itself has become a burden (often breaks because of small
format changes elsewhere).
Eventually, the syntax and types2 packages will provide most error
and diagnostic compiler output at which point fmt.go can be further
simplified as well.
Change-Id: Ie93eefd3e1166f3548fed0199b732dbd6c81948a
Reviewed-on: https://go-review.googlesource.com/c/go/+/282560
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Currently we rely on the type-checker to do some basic data-flow
analysis to help decide whether function literals should capture
variables by value or reference. However, this analysis isn't done by
go/types, and escape analysis already has a better framework for doing
this more precisely.
This CL extends escape analysis to recalculate the same "byval" as
CaptureVars and check that it matches. A future CL will remove
CaptureVars in favor of escape analysis's calculation.
Notably, escape analysis happens after deadcode removes obviously
unreachable code, so it sees the AST without any unreachable
assignments. (Also without unreachable addrtakens, but
ComputeAddrtaken already happens after deadcode too.) There are two
test cases where a variable is only reassigned on certain CPUs. This
CL changes them to reassign the variables unconditionally (as no-op
reassignments that avoid triggering cmd/vet's self-assignment check),
at least until we remove CaptureVars.
Passes toolstash -cmp.
Change-Id: I7162619739fedaf861b478fb8d506f96a6ac21f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/281535
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Tue, 5 Jan 2021 19:53:00 +0000 (11:53 -0800)]
[dev.regabi] cmd/compile: remove race-y check in Name.Canonical
The backend doesn't synchronize compilation of functions with their
enclosed function literals, so it's not safe to double-check that
IsClosureVar isn't set on the underlying Name. Plenty of frontend
stuff would blow-up if this was wrong anyway, so it should be fine to
omit.
Change-Id: I3e97b64051fe56d97bf316c9b5dcce61f2082428
Reviewed-on: https://go-review.googlesource.com/c/go/+/281812
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Matthew Dempsky [Tue, 5 Jan 2021 11:27:46 +0000 (03:27 -0800)]
[dev.regabi] cmd/compile: make ir.StaticValue safer
ir.StaticValue currently relies on CaptureVars setting Addrtaken for
variables that are assigned within nested function literals. We want
to move that logic to escape analysis, but ir.StaticValue is used in
inlining and devirtualization, which happen before escape
analysis.
The long-term solution here is to generalize escape analysis's precise
reassignment tracking for use by other optimization passes, but for
now we just generalize ir.StaticValue to not depend on Addrtaken
anymore. Instead, it now also pays attention to OADDR nodes as well as
recurses into OCLOSURE bodies.
Passes toolstash -cmp.
Change-Id: I6114e3277fb70b235f4423d2983d0433c881f79f
Reviewed-on: https://go-review.googlesource.com/c/go/+/281540
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Tue, 5 Jan 2021 11:28:06 +0000 (03:28 -0800)]
[dev.regabi] cmd/compile: make ir.OuterValue safer
For OINDEX expressions, ir.OuterValue depends on knowing the indexee's
type. Rather than silently acting as though it's not an array, make it
loudly fail.
The only code that needs to be fixed to support this is checkassign
during typechecking, which needs to avoid calling ir.OuterValue now if
typechecking the assigned operand already failed.
Passes toolstash -cmp.
Change-Id: I935cae0dacc837202bc6b63164dc2f0a6fde005c
Reviewed-on: https://go-review.googlesource.com/c/go/+/281539
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Tue, 5 Jan 2021 02:28:55 +0000 (18:28 -0800)]
[dev.regabi] cmd/compile: simplify CaptureVars
CaptureVars is responsible for deciding whether free variables should
be captured by value or by reference, but currently it also makes up
for some of the short-comings of the frontend symbol resolution /
type-checking algorithms. These are really separate responsibilities,
so move the latter into type-checking where it fits better.
Passes toolstash -cmp.
Change-Id: Iffbd53e83846a9ca9dfb54b597450b8543252850
Reviewed-on: https://go-review.googlesource.com/c/go/+/281534
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Tue, 5 Jan 2021 06:58:24 +0000 (22:58 -0800)]
[dev.regabi] cmd/compile: fix ir.Dump for []*CaseClause, etc
Dump uses reflection to print IR nodes, and it only knew how to print
out the Nodes slice type itself. This CL adds support for printing any
slice whose element type implements Node, such as SwitchStmt and
SelectStmt's clause lists.
Change-Id: I2fd8defe11868b564d1d389ea3cd9b8abcefac62
Reviewed-on: https://go-review.googlesource.com/c/go/+/281537
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 3 Jan 2021 22:37:06 +0000 (14:37 -0800)]
[dev.regabi] cmd/compile: fix re-export of parameters
When exporting signature types, we include the originating package,
because it's exposed via go/types's API. And as a consistency check,
we ensure that the parameter names came from that same package.
However, we were getting this wrong in the case of exported variables
that were initialized with a method value using an imported method. In
this case, when we created the method value wrapper function's
type (which is reused as the variable's type if none is explicitly
provided in the variable declaration), we were reusing the
original (i.e., imported) parameter names, but the newly created
signature type was associated with the current package instead.
The correct fix here is really to preserve the original signature
type's package (along with position and name for its parameters), but
that's awkward to do at the moment because the DeclFunc API requires
an ir representation of the function signature, whereas we only
provide a way to explicitly set packages via the type constructor
APIs.
As an interim fix, we associate the parameters with the current
package, to be consistent with the signature type's package.
Fixes #43479.
Change-Id: Id45a10f8cf64165c9bc7d9598f0a0ee199a5e752
Reviewed-on: https://go-review.googlesource.com/c/go/+/281292
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 3 Jan 2021 22:24:25 +0000 (14:24 -0800)]
[dev.regabi] cmd/compile: fix ICE due to large uint64 constants
It's an error to call Int64Val on constants that don't fit into
int64. CL 272654 made the compiler stricter about detecting misuse,
and revealed that we were using it improperly in detecting consecutive
integer-switch cases. That particular usage actually did work in
practice, but it's easy and best to just fix it.
Fixes #43480.
Change-Id: I56f722d75e83091638ac43b80e45df0b0ad7d48d
Reviewed-on: https://go-review.googlesource.com/c/go/+/281272
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 3 Jan 2021 08:16:46 +0000 (00:16 -0800)]
[dev.regabi] cmd/compile: fix map assignment order
After the previous cleanup/optimization CLs, ascompatee now correctly
handles map assignments too. So remove the code from order.mapAssign,
which causes us to assign to the map at the wrong point during
execution. It's not every day you get to fix an issue by only removing
code.
Thanks to Cuong Manh Le for test cases and continually following up on
this issue.
Passes toolstash -cmp. (Apparently the standard library never uses
tricky map assignments. Go figure.)
Fixes #23017.
Change-Id: Ie0728103d59d884d00c1c050251290a2a46150f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/281172
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
After the previous two optimization CLs, this code path now generates
the same code as ascompatee does anyway. So just use that and remove
some redundant code.
Passes toolstash -cmp.
Change-Id: I5e2e5c6dbea64d8e91abe0f2cf51aa5bb86576d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/281154
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 3 Jan 2021 07:56:20 +0000 (23:56 -0800)]
[dev.regabi] cmd/compile: improve walkReturn common case
Instead of evaluating all result expressions up front and then
assigning them to their result destinations, we can interleave
evaluation with assignment. This reduces how much temporary
stack/register space is needed to hold the values in flight.
Doesn't pass toolstash -cmp, because it allows better return statement
code to be generated. E.g., cmd/go's text segment on linux/ppc64le
shrinks another 1kB.
Change-Id: I3fe889342c80e947e0118704ec01f1682c577e6e
Reviewed-on: https://go-review.googlesource.com/c/go/+/281153
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sun, 3 Jan 2021 07:24:16 +0000 (23:24 -0800)]
[dev.regabi] cmd/compile: improve ascompatee
order.go has already ordered function calls, so ascompatee only needs
to worry about expressions that might access a variable after it's
already been re-assigned. It already handles this, so the safeExpr
calls simply result in unnecessarily pessimistic code.
Does not pass toolstash -cmp, because it allows more efficient code
generation. E.g., cmd/go on linux/ppc64le is about 2kB smaller.
Change-Id: Idde0588eabe7850fa13c4e281fc46bbeffb4f68c
Reviewed-on: https://go-review.googlesource.com/c/go/+/281152
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sat, 2 Jan 2021 11:15:14 +0000 (03:15 -0800)]
[dev.regabi] cmd/compile: tighten typecheckdef to *ir.Name
We only actually care about ir.Names in typecheckdef, so don't bother
calling it on anything else. Allows us to get rid of some more
superfluous .Name() calls and .(*ir.Name) assertions.
Passes toolstash -cmp.
Change-Id: I78c7cb680178991ea185958b47a36f101d4d5ef7
Reviewed-on: https://go-review.googlesource.com/c/go/+/281004
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
To do closure conversion during escape analysis, we need to walk the
AST in order. So this CL makes a few changes:
1. Function literals are walked where they appear in their enclosing
function, rather than as independent functions.
2. Walking "range" and "switch" statements is reordered to visit the
X/Tag expression up front, before the body.
3. Most assignments are refactored to use a new assignList helper,
which handles 1:1, 2:1, and N:N assignments. N:1 function call
assignments are still handled directly by the OAS2FUNC case.
4. A latent missed-optimization in escape.addr is fixed: the
ONAMEOFFSET case was failing to update k with the result of calling
e.addr(n.Name_). In partice, this probably wasn't an issue because
ONAMEOFFSET is likely only used for PEXTERN variables (which are
treated as heap memory anyway) or code generated by walk (which has
already gone through escape analysis).
5. Finally, don't replace k with discardHole at the end of
escape.addr. This is already handled at the start of escape.expr, and
we'll want to be able to access the hole's location after escape.expr
returns.
Passes toolstash -cmp.
Change-Id: I2325234346b12b10056a360c489692bab8fdbd93
Reviewed-on: https://go-review.googlesource.com/c/go/+/281003
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Sat, 2 Jan 2021 09:04:19 +0000 (01:04 -0800)]
[dev.regabi] cmd/compile: remove {Ptr,Set}Init from Node interface
This CL separates out PtrInit and SetInit into a new InitNode
extension interface, and adds a new TakeInit helper function for
taking and clearing the Init list (if any) from a Node.
This allows removing miniNode.SetInit and miniNode.PtrInit, which in
turn allow getting rid of immutableEmptyNodes, and will allow
simplification of the Nodes API.
It would be nice to get rid of the default Init method too, but
there's way more code that expects to be able to call that at the
moment, so that'll have to wait.
Passes toolstash -cmp.
Change-Id: Ia8c18fab9555b774376f7f43eeecfde4f07b5946
Reviewed-on: https://go-review.googlesource.com/c/go/+/281001
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Cuong Manh Le [Fri, 1 Jan 2021 17:39:14 +0000 (00:39 +0700)]
[dev.regabi] cmd/compile: refactor redundant type conversion [generated]
Passes toolstash -cmp.
[git-generate]
cd src/cmd/compile
rf '
ex . '"$(printf '%s\n' ./internal/* | paste -sd' ')"' {
type T interface{}
var t T
strict t
t.(T) -> t
}
'
cd internal/ir
go generate
Change-Id: I492d50390e724a7216c3cd8b49d4aaf7d0c335da
Reviewed-on: https://go-review.googlesource.com/c/go/+/280716
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Matthew Dempsky [Fri, 1 Jan 2021 12:51:22 +0000 (04:51 -0800)]
[dev.regabi] test: add another closure test case
When deciding whether a captured variable can be passed by value, the
compiler is sensitive to the order that the OCLOSURE node is
typechecked relative to the order that the variable is passed to
"checkassign". Today, for an assignment like:
q, g = 2, func() int { return q }
we get this right because we always typecheck the full RHS expression
list before calling checkassign on any LHS expression.
But I nearly made a change that would interleave this ordering,
causing us to call checkassign on q before typechecking the function
literal. And alarmingly, there weren't any tests that caught this.
So this commit adds one.
Change-Id: I66cacd61066c7a229070861a7d973bcc434904cc
Reviewed-on: https://go-review.googlesource.com/c/go/+/280998
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Fri, 1 Jan 2021 11:57:21 +0000 (03:57 -0800)]
[dev.regabi] cmd/compile: split escape analysis state
In a future CL, I plan to change escape analysis to walk function
literal bodies at the point they appear within the AST, rather than
separately as their own standalone function declaration. This means
escape analysis's AST-walking code will become reentrant.
To make this easier to get right, this CL splits escape analysis's
state into two separate types: one that holds all of the state shared
across the entire batch, and another that holds only the state that's
used within initFunc and walkFunc.
Incidentally, this CL reveals that a bunch of logopt code was using
e.curfn outside of the AST-walking code paths where it's actually set,
so it was always nil. That code is in need of refactoring anyway, so
I'll come back and figure out the correct values to pass later when I
address that.
Passes toolstash -cmp.
Change-Id: I1d13f47d06f7583401afa1b53fcc5ee2adaea6c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/280997
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Fri, 1 Jan 2021 10:39:00 +0000 (02:39 -0800)]
[dev.regabi] cmd/compile: simplify inlining of closures
Closures have their own ONAMEs for captured variables, which their
function bodies refer to. So during inlining, we need to account for
this and ensure the references still work.
The previous inlining handled this by actually declaring the variables
and then either copying the original value or creating a pointer to
them, as appropriate for variables captured by value or by reference.
But this is needlessly complicated. When inlining the function body,
we need to rewrite all variable references anyway. We can just detect
closure variables and change them to directly point to the enclosing
function's version of this variable. No need for copying or further
indirection.
Does not pass toolstash -cmp. Presumably because we're able to
generate better code in some circumstances.
Change-Id: I8f0ccf7b098f39b8cd33f3bcefb875c8132d2c62
Reviewed-on: https://go-review.googlesource.com/c/go/+/280996
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
The previous code was way overcomplicating things. To find out if a
variable is a closure pseudo-variable, one only needs to check
IsClosureVar. Checking Captured and Byval are only meant to be used by
closure conversion.
Passes toolstash -cmp.
Change-Id: I22622cba36ba7f60b3275d17999a8b6bb7c6719a
Reviewed-on: https://go-review.googlesource.com/c/go/+/280995
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
This CL moves the general deadcode-removal pass to before computing
Addrtaken, which allows variables to still be converted to SSA if
their address is only taken in unreachable code paths (e.g., the "&mp"
expression in the "if false" block in runtime/os_linux.go:newosproc).
This doesn't pass toolstash -cmp, because it allows SSA to better
optimize some code.
Change-Id: I43e54acc02fdcbad8eb6493283f355aa1ee0de84
Reviewed-on: https://go-review.googlesource.com/c/go/+/280992
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Matthew Dempsky [Fri, 1 Jan 2021 07:45:36 +0000 (23:45 -0800)]
[dev.regabi] cmd/compile: fix package-initialization order
This CL fixes package initialization order by creating the init task
before the general deadcode-removal pass.
It also changes noder to emit zero-initialization assignments (i.e.,
OAS with nil RHS) for package-block variables, so that initOrder can
tell the variables still need initialization. To allow this, we need
to also extend the static-init code to recognize zero-initialization
assignments.
This doesn't pass toolstash -cmp, because it reorders some package
initialization routines.
Fixes #43444.
Change-Id: I0da7996a62c85e15e97ce965298127e075390a7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/280976
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This commit splits up typecheck.Package and moves the code
elsewhere. The type-checking code is moved into noder, so that it can
eventually be interleaved with the noding process. The
non-type-checking code is moved back into package gc, so that it can
be incorporated into appropriate compiler backend phases.
While here, deadcode removal is moved into its own package.
Passes toolstash -cmp.
[git-generate]
cd src/cmd/compile/internal/typecheck
: Split into two functions.
sed -i -e '/Phase 6/i}\n\nfunc postTypecheck() {' typecheck.go
# Move into new package.
mv Deadcode deadcodeslice deadcodeexpr deadcode.go
mv deadcode.go cmd/compile/internal/deadcode
# Move top-level type-checking code into noder.
# Move DeclVars there too, now that nothing else uses it.
mv DeclVars Package noder.go
mv noder.go cmd/compile/internal/noder
# Move non-type-checking code back into gc.
mv postTypecheck main.go
mv main.go cmd/compile/internal/gc
'
Matthew Dempsky [Fri, 1 Jan 2021 05:32:52 +0000 (21:32 -0800)]
[dev.regabi] cmd/compile: report unused variables during typecheck
Unused variables are a type-checking error, so they should be reported
during typecheck rather than walk.
One catch is that we only want to report unused-variable errors for
functions that type check successfully, but some errors are reported
during noding, so we don't have an easy way to detect that
currently. As an approximate solution, we simply check if we've
reported any errors yet.
Passes toolstash -cmp.
Change-Id: I9400bfc94312c71d0c908a491e85c16d62224c9c
Reviewed-on: https://go-review.googlesource.com/c/go/+/280973
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>