The type syntax is reused to stand in for the actual type once typechecked,
to avoid updating all the possible references to the original type syntax.
So all these implementations allow changing their Op from the raw syntax
like OTMAP to the finished form OTYPE, even though obviously the
representation does not change.
Passes buildall w/ toolstash -cmp.
Change-Id: I4acca1a5b35fa2f48ee08e8f1e5a330a004c284b
Reviewed-on: https://go-review.googlesource.com/c/go/+/274103
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
This is a general operation on IR nodes, so it belongs in ir.
The copied implementation is adapted to support the
extension pattern, allowing nodes to implement their
own DeepCopy implementations if needed.
This is the first step toward higher-level operations instead
of Left, Right, etc. It will allow the new type syntax nodes
to be properly immutable and opt out of those fine-grained methods.
Passes buildall w/ toolstash -cmp.
Change-Id: Ibd64061e01daf14aebc6586cb2eb2b12057ca85a
Reviewed-on: https://go-review.googlesource.com/c/go/+/274102
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Now that we have specific types for ONAME and ODCLFUNC nodes
(*Name and *Func), use them throughout the compiler to be more
precise about what data is being operated on.
This is a somewhat large CL, but once you start applying the types
in a few places, you end up needing to apply them to many other
places to keep everything type-checking. A lot of code also melts
away as types are added.
Passes buildall w/ toolstash -cmp.
Change-Id: I21dd9b945d701c470332bac5394fca744a5b232d
Reviewed-on: https://go-review.googlesource.com/c/go/+/274097
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Before this CL, an ONAME Node was represented by three structs
linked together: a node, a Name, and a Param. Previous CLs removed
OLABEL and OPACK from the set of nodes that knew about Name.
Now Name can be repurposed to *be* the ONAME Node implementation,
replacing three linked structs totaling 152+64+88 = 304 bytes (64-bit)
with a single 232-byte struct.
Many expressions in the code become simpler as well, without having
to use .Param. and sometimes even .Name().
(For a node n where n.Name() != nil, n.Name() == n.(*Name) now.)
Passes buildall w/ toolstash -cmp.
Change-Id: Ie719f1285c05623b9fd2faaa059e5b360a64b3be
Reviewed-on: https://go-review.googlesource.com/c/go/+/274094
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
OPACK was using a whole Node and Name and Param
to hold about three fields. Give it its own implementation.
Passes buildall w/ toolstash -cmp.
Change-Id: I85a28b43d37183b2062d337b0b1b2eea52884e8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/274093
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
These are only needed for a few opcodes, and we can avoid
wasting storage in every implementation by using the extension
interface pattern with a helper function for access.
Of course, in the current codebase, there is only one Node
implementation (*node) and it has these methods, so there
is no danger of a functional change in this particular CL.
Passes buildall w/ toolstash -cmp.
Change-Id: I440c6c232f1fe7b56b852a00dc530f8f49a6b12d
Reviewed-on: https://go-review.googlesource.com/c/go/+/274089
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
- The use of a label's Name.Defn to point at the named for/select/switch
means that any rewrite of the for/select/switch must overwrite the original
or else the pointer will dangle. Remove that pointer by adding the label
name directly to the for/select/switch representation instead.
- The only uses of a label's Sym.Label were ephemeral values during
markbreak and escape analysis. Use a map for each. Although in general
we are not going to replace all computed fields with maps (too slow),
the one in markbreak is only for labeled for/select/switch, and the one
in escape is for all labels, but even so, labels are fairly rare.
In theory this cleanup should make it easy to allow labeled for/select/switch
in inlined bodies, but this CL does not attempt that. It's only concerned
with cleanup to enable a new Node representation.
Passes buildall w/ toolstash -cmp.
Change-Id: I7e36ee98d2ea40dbae94e6722d585f007b7afcfa
Reviewed-on: https://go-review.googlesource.com/c/go/+/274086
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The plan is to introduce a Node interface that replaces the old *Node pointer-to-struct.
The previous CL defined an interface INode modeling a *Node.
This CL:
- Changes all references outside internal/ir to use INode,
along with many references inside internal/ir as well.
- Renames Node to node.
- Renames INode to Node
So now ir.Node is an interface implemented by *ir.node, which is otherwise inaccessible,
and the code outside package ir is now (clearly) using only the interface.
The usual rule is never to redefine an existing name with a new meaning,
so that old code that hasn't been updated gets a "unknown name" error
instead of more mysterious errors or silent misbehavior. That rule would
caution against replacing Node-the-struct with Node-the-interface,
as in this CL, because code that says *Node would now be using a pointer
to an interface. But this CL is being landed at the same time as another that
moves Node from gc to ir. So the net effect is to replace *gc.Node with ir.Node,
which does follow the rule: any lingering references to gc.Node will be told
it's gone, not silently start using pointers to interfaces. So the rule is followed
by the CL sequence, just not this specific CL.
Overall, the loss of inlining caused by using interfaces cuts the compiler speed
by about 6%, a not insignificant amount. However, as we convert the representation
to concrete structs that are not the giant Node over the next weeks, that speed
should come back as more of the compiler starts operating directly on concrete types
and the memory taken up by the graph of Nodes drops due to the more precise
structs. Honestly, I was expecting worse.
% benchstat bench.old bench.new
name old time/op new time/op delta
Template 168ms ± 4% 182ms ± 2% +8.34% (p=0.000 n=9+9)
Unicode 72.2ms ±10% 82.5ms ± 6% +14.38% (p=0.000 n=9+9)
GoTypes 563ms ± 8% 598ms ± 2% +6.14% (p=0.006 n=9+9)
Compiler 2.89s ± 4% 3.04s ± 2% +5.37% (p=0.000 n=10+9)
SSA 6.45s ± 4% 7.25s ± 5% +12.41% (p=0.000 n=9+10)
Flate 105ms ± 2% 115ms ± 1% +9.66% (p=0.000 n=10+8)
GoParser 144ms ±10% 152ms ± 2% +5.79% (p=0.011 n=9+8)
Reflect 345ms ± 9% 370ms ± 4% +7.28% (p=0.001 n=10+9)
Tar 149ms ± 9% 161ms ± 5% +8.05% (p=0.001 n=10+9)
XML 190ms ± 3% 209ms ± 2% +9.54% (p=0.000 n=9+8)
LinkCompiler 327ms ± 2% 325ms ± 2% ~ (p=0.382 n=8+8)
ExternalLinkCompiler 1.77s ± 4% 1.73s ± 6% ~ (p=0.113 n=9+10)
LinkWithoutDebugCompiler 214ms ± 4% 211ms ± 2% ~ (p=0.360 n=10+8)
StdCmd 14.8s ± 3% 15.9s ± 1% +6.98% (p=0.000 n=10+9)
[Geo mean] 480ms 510ms +6.31%
name old user-time/op new user-time/op delta
Template 223ms ± 3% 237ms ± 3% +6.16% (p=0.000 n=9+10)
Unicode 103ms ± 6% 113ms ± 3% +9.53% (p=0.000 n=9+9)
GoTypes 758ms ± 8% 800ms ± 2% +5.55% (p=0.003 n=10+9)
Compiler 3.95s ± 2% 4.12s ± 2% +4.34% (p=0.000 n=10+9)
SSA 9.43s ± 1% 9.74s ± 4% +3.25% (p=0.000 n=8+10)
Flate 132ms ± 2% 141ms ± 2% +6.89% (p=0.000 n=9+9)
GoParser 177ms ± 9% 183ms ± 4% ~ (p=0.050 n=9+9)
Reflect 467ms ±10% 495ms ± 7% +6.17% (p=0.029 n=10+10)
Tar 183ms ± 9% 197ms ± 5% +7.92% (p=0.001 n=10+10)
XML 249ms ± 5% 268ms ± 4% +7.82% (p=0.000 n=10+9)
LinkCompiler 544ms ± 5% 544ms ± 6% ~ (p=0.863 n=9+9)
ExternalLinkCompiler 1.79s ± 4% 1.75s ± 6% ~ (p=0.075 n=10+10)
LinkWithoutDebugCompiler 248ms ± 6% 246ms ± 2% ~ (p=0.965 n=10+8)
[Geo mean] 483ms 504ms +4.41%
[git-generate]
cd src/cmd/compile/internal/ir
: # We need to do the conversion in multiple steps, so we introduce
: # a temporary type alias that will start out meaning the pointer-to-struct
: # and then change to mean the interface.
rf '
mv Node OldNode
add node.go \
type Node = *OldNode
'
: # It should work to do this ex in ir, but it misses test files, due to a bug in rf.
: # Run the command in gc to handle gc's tests, and then again in ssa for ssa's tests.
cd ../gc
rf '
ex . ../arm ../riscv64 ../arm64 ../mips64 ../ppc64 ../mips ../wasm {
import "cmd/compile/internal/ir"
*ir.OldNode -> ir.Node
}
'
cd ../ssa
rf '
ex {
import "cmd/compile/internal/ir"
*ir.OldNode -> ir.Node
}
'
: # Back in ir, finish conversion clumsily with sed,
: # because type checking and circular aliases do not mix.
cd ../ir
sed -i '' '
/type Node = \*OldNode/d
s/\*OldNode/Node/g
s/^func (n Node)/func (n *OldNode)/
s/OldNode/node/g
s/type INode interface/type Node interface/
s/var _ INode = (Node)(nil)/var _ Node = (*node)(nil)/
' *.go
gofmt -w *.go
sed -i '' '
s/{Func{}, 136, 248}/{Func{}, 152, 280}/
s/{Name{}, 32, 56}/{Name{}, 44, 80}/
s/{Param{}, 24, 48}/{Param{}, 44, 88}/
s/{node{}, 76, 128}/{node{}, 88, 152}/
' sizeof_test.go
cd ../ssa
sed -i '' '
s/{LocalSlot{}, 28, 40}/{LocalSlot{}, 32, 48}/
' sizeof_test.go
cd ../gc
sed -i '' 's/\*ir.Node/ir.Node/' mkbuiltin.go
cd ../../../..
go install std cmd
cd cmd/compile
go test -u || go test -u
Change-Id: I196bbe3b648e4701662e4a2bada40bf155e2a553
Reviewed-on: https://go-review.googlesource.com/c/go/+/272935
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
If we want to break up package gc at all, we will need to move
the compiler IR it defines into a separate package that can be
imported by packages that gc itself imports. This CL does that.
It also removes the TINT8 etc aliases so that all code is clear
about which package things are coming from.
This CL is automatically generated by the script below.
See the comments in the script for details about the changes.
[git-generate]
cd src/cmd/compile/internal/gc
rf '
# These names were never fully qualified
# when the types package was added.
# Do it now, to avoid confusion about where they live.
inline -rm \
Txxx \
TINT8 \
TUINT8 \
TINT16 \
TUINT16 \
TINT32 \
TUINT32 \
TINT64 \
TUINT64 \
TINT \
TUINT \
TUINTPTR \
TCOMPLEX64 \
TCOMPLEX128 \
TFLOAT32 \
TFLOAT64 \
TBOOL \
TPTR \
TFUNC \
TSLICE \
TARRAY \
TSTRUCT \
TCHAN \
TMAP \
TINTER \
TFORW \
TANY \
TSTRING \
TUNSAFEPTR \
TIDEAL \
TNIL \
TBLANK \
TFUNCARGS \
TCHANARGS \
NTYPE \
BADWIDTH
# esc.go and escape.go do not need to be split.
# Append esc.go onto the end of escape.go.
mv esc.go escape.go
# Pull out the type format installation from func Main,
# so it can be carried into package ir.
mv Main:/Sconv.=/-0,/TypeLinkSym/-1 InstallTypeFormats
# Names that need to be exported for use by code left in gc.
mv Isconst IsConst
mv asNode AsNode
mv asNodes AsNodes
mv asTypesNode AsTypesNode
mv basicnames BasicTypeNames
mv builtinpkg BuiltinPkg
mv consttype ConstType
mv dumplist DumpList
mv fdumplist FDumpList
mv fmtMode FmtMode
mv goopnames OpNames
mv inspect Inspect
mv inspectList InspectList
mv localpkg LocalPkg
mv nblank BlankNode
mv numImport NumImport
mv opprec OpPrec
mv origSym OrigSym
mv stmtwithinit StmtWithInit
mv dump DumpAny
mv fdump FDumpAny
mv nod Nod
mv nodl NodAt
mv newname NewName
mv newnamel NewNameAt
mv assertRepresents AssertValidTypeForConst
mv represents ValidTypeForConst
mv nodlit NewLiteral
# Types and fields that need to be exported for use by gc.
mv nowritebarrierrecCallSym SymAndPos
mv SymAndPos.lineno SymAndPos.Pos
mv SymAndPos.target SymAndPos.Sym
mv Func.lsym Func.LSym
mv Func.setWBPos Func.SetWBPos
mv Func.numReturns Func.NumReturns
mv Func.numDefers Func.NumDefers
mv Func.nwbrCalls Func.NWBRCalls
# initLSym is an algorithm left behind in gc,
# not an operation on Func itself.
mv Func.initLSym initLSym
mv nodeQueue NodeQueue
mv NodeQueue.empty NodeQueue.Empty
mv NodeQueue.popLeft NodeQueue.PopLeft
mv NodeQueue.pushRight NodeQueue.PushRight
# Many methods on Node are actually algorithms that
# would apply to any node implementation.
# Those become plain functions.
mv Node.funcname FuncName
mv Node.isBlank IsBlank
mv Node.isGoConst isGoConst
mv Node.isNil IsNil
mv Node.isParamHeapCopy isParamHeapCopy
mv Node.isParamStackCopy isParamStackCopy
mv Node.isSimpleName isSimpleName
mv Node.mayBeShared MayBeShared
mv Node.pkgFuncName PkgFuncName
mv Node.backingArrayPtrLen backingArrayPtrLen
mv Node.isterminating isTermNode
mv Node.labeledControl labeledControl
mv Nodes.isterminating isTermNodes
mv Nodes.sigerr fmtSignature
mv Node.MethodName methodExprName
mv Node.MethodFunc methodExprFunc
mv Node.IsMethod IsMethod
# Every node will need to implement RawCopy;
# Copy and SepCopy algorithms will use it.
mv Node.rawcopy Node.RawCopy
mv Node.copy Copy
mv Node.sepcopy SepCopy
# Extract Node.Format method body into func FmtNode,
# but leave method wrapper behind.
mv Node.Format:0,$ FmtNode
# Formatting helpers that will apply to all node implementations.
mv Node.Line Line
mv Node.exprfmt exprFmt
mv Node.jconv jconvFmt
mv Node.modeString modeString
mv Node.nconv nconvFmt
mv Node.nodedump nodeDumpFmt
mv Node.nodefmt nodeFmt
mv Node.stmtfmt stmtFmt
# Constant support needed for code moving to ir.
mv okforconst OKForConst
mv vconv FmtConst
mv int64Val Int64Val
mv float64Val Float64Val
mv Node.ValueInterface ConstValue
# Organize code into files.
mv LocalPkg BuiltinPkg ir.go
mv NumImport InstallTypeFormats Line fmt.go
mv syntax.go Nod NodAt NewNameAt Class Pxxx PragmaFlag Nointerface SymAndPos \
AsNode AsTypesNode BlankNode OrigSym \
Node.SliceBounds Node.SetSliceBounds Op.IsSlice3 \
IsConst Node.Int64Val Node.CanInt64 Node.Uint64Val Node.BoolVal Node.StringVal \
Node.RawCopy SepCopy Copy \
IsNil IsBlank IsMethod \
Node.Typ Node.StorageClass node.go
mv ConstType ConstValue Int64Val Float64Val AssertValidTypeForConst ValidTypeForConst NewLiteral idealType OKForConst val.go
# Move files to new ir package.
mv bitset.go class_string.go dump.go fmt.go \
ir.go node.go op_string.go val.go \
sizeof_test.go cmd/compile/internal/ir
'
: # fix mkbuiltin.go to generate the changes made to builtin.go during rf
sed -i '' '
s/\[T/[types.T/g
s/\*Node/*ir.Node/g
/internal\/types/c \
fmt.Fprintln(&b, `import (`) \
fmt.Fprintln(&b, ` "cmd/compile/internal/ir"`) \
fmt.Fprintln(&b, ` "cmd/compile/internal/types"`) \
fmt.Fprintln(&b, `)`)
' mkbuiltin.go
gofmt -w mkbuiltin.go
: # update cmd/dist to add internal/ir
cd ../../../dist
sed -i '' '/compile.internal.gc/a\
"cmd/compile/internal/ir",
' buildtool.go
gofmt -w buildtool.go
: # update cmd/compile TestFormats
cd ../..
go install std cmd
cd cmd/compile
go test -u || go test # first one updates but fails; second passes
Change-Id: I5f7caf6b20629b51970279e81231a3574d5b51db
Reviewed-on: https://go-review.googlesource.com/c/go/+/273008
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@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>
The debug table is not as haphazard as flags, but there are still
a few mismatches between command-line names and variable names.
This CL moves them all into a consistent home (var Debug, like var Flag).
Code updated automatically using the rf command below.
A followup CL will make a few manual cleanups, leaving this CL
completely automated and easier to regenerate during merge
conflicts.
[git-generate]
cd src/cmd/compile/internal/gc
rf '
add main.go var Debug struct{}
mv Debug_append Debug.Append
mv Debug_checkptr Debug.Checkptr
mv Debug_closure Debug.Closure
mv Debug_compilelater Debug.CompileLater
mv disable_checknil Debug.DisableNil
mv debug_dclstack Debug.DclStack
mv Debug_gcprog Debug.GCProg
mv Debug_libfuzzer Debug.Libfuzzer
mv Debug_checknil Debug.Nil
mv Debug_panic Debug.Panic
mv Debug_slice Debug.Slice
mv Debug_typeassert Debug.TypeAssert
mv Debug_wb Debug.WB
mv Debug_export Debug.Export
mv Debug_pctab Debug.PCTab
mv Debug_locationlist Debug.LocationLists
mv Debug_typecheckinl Debug.TypecheckInl
mv Debug_gendwarfinl Debug.DwarfInl
mv Debug_softfloat Debug.SoftFloat
mv Debug_defer Debug.Defer
mv Debug_dumpptrs Debug.DumpPtrs
mv flag.go:/parse.-d/-1,/unknown.debug/+2 parseDebug
mv debugtab Debug parseDebug \
debugHelpHeader debugHelpFooter \
debug.go
# Remove //go:generate line copied from main.go
rm debug.go:/go:generate/-+
'
Change-Id: I625761ca5659be4052f7161a83baa00df75cca91
Reviewed-on: https://go-review.googlesource.com/c/go/+/272246
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The original meaning of type Func was "extra fields factored out
of a few cases of type Node having to do with functions",
but those specific cases didn't necessarily have any relation.
A typical declared function is represented by an ODCLFUNC Node
at its declaration and an ONAME node at its uses, and both those
have a .Func field, but they are *different* Funcs.
Similarly, a closure is represented both by an OCLOSURE Node for
the value itself and an ODCLFUNC Node for the underlying function
implementing the closure. Those too have *different* Funcs,
and the Func.Closure field in one points to the other and vice versa.
This has led to no end of confusion over the years.
This CL elevates type Func to be the canonical identifier for
a given Go function.
This looks like a trivial CL but in fact is the result of a lot of
scaffolding and rewriting, discarded once the result was achieved, to
separate out the three different kinds of Func nodes into three
separate fields, limited in use to each specific Node type, to
understand which Func fields are used by which Node types and what the
possible overlaps are. There were a few overlaps, most notably around
closures, which led to more fields being added to type Func to keep
them separate even though there is now a single Func instead of two
different ones for each function.
A future CL can and should change Curfn to be a *Func instead of
a *Node, finally eliminating the confusion about whether Curfn
is an ODCLFUNC node (as it is most of the time) or an ONAME node
(as it is when type-checking an inlined function body).
Although sizeof_test.go makes it look like Func is growing by two
words, there are now half as many Funcs in a running compilation,
so the memory footprint has actually been reduced substantially.
Change-Id: I598bd96c95728093dc769a835d48f2154a406a61
Reviewed-on: https://go-review.googlesource.com/c/go/+/272253
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Properly speaking, "nil" is a zero value, not a constant. So
go/constant does not have a representation for it. To allow replacing
Val with constant.Value, we split out ONIL separately from OLITERAL so
we can get rid of CTNIL.
Passes toolstash-check.
Change-Id: I4c8e60cae3b3c91bbac43b3b0cf2a4ade028d6cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/272650
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
The next CL will introduce a package ir to hold the IR definitions.
This CL adjusts a few names and makes a few other minor changes
to make the next CL - an automated one - smoother.
Change-Id: Ie787a34732efd5b3d171bf0c1220b6dd91994ce3
Reviewed-on: https://go-review.googlesource.com/c/go/+/272251
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Prepare for factoring the error API out of this package by
cleaning it up. The doc comments use the intended new names,
which will be introduced in the next CL.
Change-Id: Ie4c8d4262422da32a9a9f750fda42c225b6b42a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/272248
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
pointers to go:notinheap types should be treated as scalars. That
means they shouldn't be stored directly in interfaces, or directly
in reflect.Value.ptr.
Also be sure to use uintpr to compare such pointers in reflect.DeepEqual.
Fixes#42076
Change-Id: I53735f6d434e9c3108d4940bd1bae14c61ef2a74
Reviewed-on: https://go-review.googlesource.com/c/go/+/264480
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gc debug flags are currently stored in a 256-long array, that is then
addressed using the ASCII numeric value of the flag itself (a quirk
inherited from the old C compiler). It is also a little wasteful,
since we only define 16 flags, and the other 240 array elements are
always empty.
This change makes Debug a struct, which also provides static checking
that we're not referencing flags that does not exist.
Change-Id: I2f0dfef2529325514b3398cf78635543cdf48fe0
Reviewed-on: https://go-review.googlesource.com/c/go/+/263539
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
On a negative answer, the assignop and convertop functions write the
reason why to a string pointer passed as an argument, likely a C-ism
leftover since the compiler's machine assisted translation to Go.
This change makes why a return parameter.
It also fixes a few places where the assignop/convertop result was
compared to 0. While OXXX's value may be zero now, using the named
constant is more robust.
Change-Id: Id9147ed4c1b97d658d30a2f778f876b7867006b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/261857
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Change-Id: I576a596ed8e9ce14e3750031d0e338e9276eff1e
Reviewed-on: https://go-review.googlesource.com/c/go/+/262537
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
To be consistent with go/types.
Passes toolstash-check.
Change-Id: I5e02f529064a904310a164f8765082aa533cc799
Reviewed-on: https://go-review.googlesource.com/c/go/+/260699
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>
Similar to how we report initialization loops in initorder.go and type
alias loops in typecheck.go, this CL updates align.go to warn about
invalid recursive types. The code is based on the loop code from
initorder.go, with minimal changes to adapt from detecting
variable/function initialization loops to detecting type declaration
loops.
Thanks to Cuong Manh Le for investigating this, helping come up with
test cases, and exploring solutions.
Fixes#41575
Updates #41669.
Change-Id: Idb2cb8c5e1d645e62900e178fcb50af33e1700a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/258177
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Update #40954
Change-Id: Ifaab7349631ccb12fc892882bbdf7f0ebf3d845f
Reviewed-on: https://go-review.googlesource.com/c/go/+/251158
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Use a common runtime slicecopy function to copy strings or slices
into slices. This deduplicates similar code previously used in
reflect.slicecopy and runtime.stringslicecopy.
Change-Id: I09572ff0647a9e12bb5c6989689ce1c43f16b7f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/254658
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Currently, there's awkward reentrancy issue with funccompile:
funccompile -> compile -> dtypesym -> geneq/genhash/genwrapper -> funccompile
Though it's not a problem at this moment, some attempts by @mdempsky to
move order/walk/instrument into buildssa was failed, due to SSA cache
corruption.
This commit fixes that reentrancy issue, by making generated functions
to be pumped through the same compile workqueue that normal functions
are compiled. We do this by adding them to xtop, instead of calling
funccompile directly in geneq/genhash/genwrapper. In dumpdata, we look
for uncompiled functions in xtop instead of compilequeue, then finish
compiling them.
Updates #38463Fixes#33485
Change-Id: Ic9f0ce45b56ae2ff3862f17fd979253ddc144bb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/254617
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
The primary responsibility of declare() to associate a symbol (Sym) with
a declaration (Node), so "oldname" will work. Function literals are
anonymous, so their symbols does not need to be declared.
Passes toolstash-check.
Change-Id: I739b1054e3953e85fbd74a99148b9cfd7e5a57eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/249078
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Omits printing the file:line:column when trying to
open non-existent files
Given:
go tool compile x.go
* Before:
x.go:0: open x.go: no such file or directory
* After:
open x.go: no such file or directory
Reverts the revert in CL 231043 by only fixing the case
of non-existent errors which is what the original bug
was about. The fix for "permission errors" will come later
on when I have bandwidth to investigate the differences
between running with root and why os.Open works for some
builders and not others.
Fixes#36437
Change-Id: I9c8a0981ad708b504bb43990a4105b42266fa41f
Reviewed-on: https://go-review.googlesource.com/c/go/+/230941
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
This reverts commit 4f7053c87f.
Reason for revert: Newly added test is failing on several builders.
Change-Id: I22dcbfebf2f57735b2f479886bbeb623f95b132f
Reviewed-on: https://go-review.googlesource.com/c/go/+/231043
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Omits printing the file:line:column when trying to open either
* non-existent files
* files without permission
Given:
go tool compile x.go
For either of x.go not existing, or if no read permissions:
* Before:
x.go:0: open x.go: no such file or directory
x.go:0: open x.go: permission denied
* After:
open x.go: no such file or directory
open x.go: permission denied
While here, noticed an oddity with the Linux builders, that appear
to always be running under root, hence the test for permission errors
with 0222 -W-*-W-*-W- can't pass on linux-amd64 builders.
The filed bug is #38608.
Fixes#36437
Change-Id: I9645ef73177c286c99547e3a0f3719fa07b35cb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/229357
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Node.NonNil and Node.Bounded were a bit muddled. This led to #38496.
This change clarifies and documents them.
It also corrects one misuse.
However, since ssa conversion doesn't make full use of the bounded hint,
this correction doesn't change any generated code.
The next change will fix that.
Passes toolstash-check.
Change-Id: I2bcd487a0a4aef5d7f6090e653974fce0dce3b8e
Reviewed-on: https://go-review.googlesource.com/c/go/+/228787
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This lets us provide a better position in its use in swt.go.
Change-Id: I7c0da6bd0adea81acfc9a591e6a01b241a5e0942
Reviewed-on: https://go-review.googlesource.com/c/go/+/228219
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Consider
switch x:= x.(type) {
case int:
// int stmts
case error:
// error stmts
}
Prior to this change, we lowered this roughly as:
if x, ok := x.(int); ok {
// int stmts
} else if x, ok := x.(error); ok {
// error stmts
}
x, ok := x.(error) is implemented with a call to runtime.assertE2I2 or runtime.assertI2I2.
x, ok := x.(int) generates inline code that checks whether x has type int,
and populates x and ok as appropriate. We then immediately branch again on ok.
The shortcircuit pass in the SSA backend is designed to recognize situations
like this, in which we are immediately branching on a bool value
that we just calculated with a branch.
However, the shortcircuit pass has limitations when the intermediate state has phis.
In this case, the phi value is x (the int).
CL 222923 improved the situation, but many cases are still unhandled.
I have further improvements in progress, which is how I found this particular problem,
but they are expensive, and may or may not see the light of day.
In the common case of a lone concrete type in a type switch case,
it is easier and cheaper to simply lower a different way, roughly:
if _, ok := x.(int); ok {
x := x.(int)
// int stmts
}
Instead of using a type assertion, though, we extract the value of x
from the interface directly.
This removes the need to track x (the int) across the branch on ok,
which removes the phi, which lets the shortcircuit pass do its job.
Benchmarks for encoding/binary show improvements, as well as some
wild swings on the super fast benchmarks (alignment effects?):
name old time/op new time/op delta
ReadSlice1000Int32s-8 5.25µs ± 2% 4.87µs ± 3% -7.11% (p=0.000 n=44+49)
ReadStruct-8 451ns ± 2% 417ns ± 2% -7.39% (p=0.000 n=45+46)
WriteStruct-8 412ns ± 2% 405ns ± 3% -1.58% (p=0.000 n=46+48)
ReadInts-8 296ns ± 8% 275ns ± 3% -7.23% (p=0.000 n=48+50)
WriteInts-8 324ns ± 1% 318ns ± 2% -1.67% (p=0.000 n=44+49)
WriteSlice1000Int32s-8 5.21µs ± 2% 4.92µs ± 1% -5.67% (p=0.000 n=46+44)
PutUint16-8 0.58ns ± 2% 0.59ns ± 2% +0.63% (p=0.000 n=49+49)
PutUint32-8 0.87ns ± 1% 0.58ns ± 1% -33.10% (p=0.000 n=46+44)
PutUint64-8 0.66ns ± 2% 0.87ns ± 2% +33.07% (p=0.000 n=47+48)
LittleEndianPutUint16-8 0.86ns ± 2% 0.87ns ± 2% +0.55% (p=0.003 n=47+50)
LittleEndianPutUint32-8 0.87ns ± 1% 0.87ns ± 1% ~ (p=0.547 n=45+47)
LittleEndianPutUint64-8 0.87ns ± 2% 0.87ns ± 1% ~ (p=0.451 n=46+47)
ReadFloats-8 79.8ns ± 5% 75.9ns ± 2% -4.83% (p=0.000 n=50+47)
WriteFloats-8 89.3ns ± 1% 88.9ns ± 1% -0.48% (p=0.000 n=46+44)
ReadSlice1000Float32s-8 5.51µs ± 1% 4.87µs ± 2% -11.74% (p=0.000 n=47+46)
WriteSlice1000Float32s-8 5.51µs ± 1% 4.93µs ± 1% -10.60% (p=0.000 n=48+47)
PutUvarint32-8 25.9ns ± 2% 24.0ns ± 2% -7.02% (p=0.000 n=48+50)
PutUvarint64-8 75.1ns ± 1% 61.5ns ± 2% -18.12% (p=0.000 n=45+47)
[Geo mean] 57.3ns 54.3ns -5.33%
Despite the rarity of type switches, this generates noticeably smaller binaries.
file before after Δ %
addr2line 4413296 4409200 -4096 -0.093%
api 5982648 5962168 -20480 -0.342%
cgo 4854168 4833688 -20480 -0.422%
compile 19694784 19682560 -12224 -0.062%
cover 5278008 5265720 -12288 -0.233%
doc 4694824 4682536 -12288 -0.262%
fix 3411336 3394952 -16384 -0.480%
link 6721496 6717400 -4096 -0.061%
nm 4371152 4358864 -12288 -0.281%
objdump 4760960 4752768 -8192 -0.172%
pprof 14810820 14790340 -20480 -0.138%
trace 11681076 11668788 -12288 -0.105%
vet 8285464 8244504 -40960 -0.494%
total 115824120 115627576 -196544 -0.170%
Compiler performance is marginally improved (note that go/types has many type switches):
name old alloc/op new alloc/op delta
Template 35.0MB ± 0% 35.0MB ± 0% +0.09% (p=0.008 n=5+5)
Unicode 28.5MB ± 0% 28.5MB ± 0% ~ (p=0.548 n=5+5)
GoTypes 114MB ± 0% 114MB ± 0% -0.76% (p=0.008 n=5+5)
Compiler 541MB ± 0% 541MB ± 0% -0.03% (p=0.008 n=5+5)
SSA 1.17GB ± 0% 1.17GB ± 0% ~ (p=0.841 n=5+5)
Flate 21.9MB ± 0% 21.9MB ± 0% ~ (p=0.421 n=5+5)
GoParser 26.9MB ± 0% 26.9MB ± 0% ~ (p=0.222 n=5+5)
Reflect 74.6MB ± 0% 74.6MB ± 0% ~ (p=1.000 n=5+5)
Tar 32.9MB ± 0% 32.8MB ± 0% ~ (p=0.056 n=5+5)
XML 42.4MB ± 0% 42.1MB ± 0% -0.77% (p=0.008 n=5+5)
[Geo mean] 73.2MB 73.1MB -0.15%
name old allocs/op new allocs/op delta
Template 377k ± 0% 377k ± 0% +0.06% (p=0.008 n=5+5)
Unicode 354k ± 0% 354k ± 0% ~ (p=0.095 n=5+5)
GoTypes 1.31M ± 0% 1.30M ± 0% -0.73% (p=0.008 n=5+5)
Compiler 5.44M ± 0% 5.44M ± 0% -0.04% (p=0.008 n=5+5)
SSA 11.7M ± 0% 11.7M ± 0% ~ (p=1.000 n=5+5)
Flate 239k ± 0% 239k ± 0% ~ (p=1.000 n=5+5)
GoParser 302k ± 0% 302k ± 0% -0.04% (p=0.008 n=5+5)
Reflect 977k ± 0% 977k ± 0% ~ (p=0.690 n=5+5)
Tar 346k ± 0% 346k ± 0% ~ (p=0.889 n=5+5)
XML 431k ± 0% 430k ± 0% -0.25% (p=0.008 n=5+5)
[Geo mean] 806k 806k -0.10%
For packages with many type switches, this considerably shrinks function text size.
Some examples:
file before after Δ %
encoding/binary.s 30726 29504 -1222 -3.977%
go/printer.s 77597 76005 -1592 -2.052%
cmd/vendor/golang.org/x/tools/go/ast/astutil.s 65704 63318 -2386 -3.631%
cmd/vendor/golang.org/x/tools/go/analysis/passes/unreachable.s 8047 7714 -333 -4.138%
Text size regressions are rare.
Change-Id: Ic10982bbb04876250eaa5bfee97990141ae5fc28
Reviewed-on: https://go-review.googlesource.com/c/go/+/228106
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Some runtime calls accept a slice, but only use ptr and len.
This change modifies most such routines to accept only ptr and len.
After this change, the only runtime calls that accept an unnecessary
cap arg are concatstrings and slicerunetostring.
Neither is particularly common, and both are complicated to modify.
Negligible compiler performance impact. Shrinks binaries a little.
There are only a few regressions; the one I investigated was
due to register allocation fluctuation.
Passes 'go test -race std cmd', modulo #38265 and #38266.
Wow, does that take a long time to run.
Updates #36890
file before after Δ %
compile 19655024 19655152 +128 +0.001%
cover 5244840 5236648 -8192 -0.156%
dist 3662376 3658280 -4096 -0.112%
link 6680056 6675960 -4096 -0.061%
pprof 14789844 14777556 -12288 -0.083%
test2json 2824744 2820648 -4096 -0.145%
trace 11647876 11639684 -8192 -0.070%
vet 8260472 8256376 -4096 -0.050%
total 115163736 115118808 -44928 -0.039%
Change-Id: Idb29fa6a81d6a82bfd3b65740b98cf3275ca0a78
Reviewed-on: https://go-review.googlesource.com/c/go/+/227163
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
In CL 187657, I refactored constant conversion logic without realizing
that conversions between int/float and complex types are allowed for
constants (assuming the constant values are representable by the
destination type), but are never allowed for non-constant expressions.
This CL expands convertop to take an extra srcConstant parameter to
indicate whether the source expression is a constant; and if so, to
allow any numeric-to-numeric conversion. (Conversions of values that
cannot be represented in the destination type are rejected by
evconst.)
Fixes#38117.
Change-Id: Id7077d749a14c8fd910be38da170fa5254819f2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/226197
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
This flag is supposed to indicate whether the expression is
"addressable"; but in practice, we infer this from other
attributes about the expression (e.g., n.Op and n.Class()).
Passes toolstash-check.
Change-Id: I19352ca07ab5646e232d98e8a7c1c9aec822ddd0
Reviewed-on: https://go-review.googlesource.com/c/go/+/200897
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This CL detangles the hairy mess that was convlit+defaultlit. In
particular, it makes the following changes:
1. convlit1 now follows the standard typecheck behavior of setting
"n.Type = nil" if there's an error. Notably, this means for a lot of
test cases, we now avoid reporting useless follow-on error messages.
For example, after reporting that "1 << s + 1.0" has an invalid shift,
we no longer also report that it can't be assigned to string.
2. Previously, assignconvfn had some extra logic for trying to
suppress errors from convlit/defaultlit so that it could provide its
own errors with better context information. Instead, this extra
context information is now passed down into convlit1 directly.
3. Relatedly, this CL also removes redundant calls to defaultlit prior
to assignconv. As a consequence, when an expression doesn't make sense
for a particular assignment (e.g., assigning an untyped string to an
integer), the error messages now say "untyped string" instead of just
"string". This is more consistent with go/types behavior.
4. defaultlit2 is now smarter about only trying to convert pairs of
untyped constants when it's likely to succeed. This allows us to
report better error messages for things like 3+"x"; instead of "cannot
convert 3 to string" we now report "mismatched types untyped number
and untyped string".
Passes toolstash-check.
Change-Id: I26822a02dc35855bd0ac774907b1cf5737e91882
Reviewed-on: https://go-review.googlesource.com/c/go/+/187657
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Drops support for old escape analysis pass. Subsequent, separate CL
will remove dead code.
While here, fix a minor error in fmt.go: it was still looking for
esc.go's NodeEscState in n.Opt() rather than escape.go's EscLocation.
But this only affected debug diagnostics printed during escape
analysis itself.
Change-Id: I62512e1b31c75ba0577550a5fd7824abc3159ed5
Reviewed-on: https://go-review.googlesource.com/c/go/+/187597
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Because the Node AST represents references to declared objects (e.g.,
variables, packages, types, constants) by directly pointing to the
referred object, we don't have use-position info for these objects.
For switch statements with duplicate cases, we report back where the
first duplicate value appeared. However, due to the AST
representation, if the value was a declared constant, we mistakenly
reported the constant declaration position as the previous case
position.
This CL reports back against the 'case' keyword's position instead, if
there's no more precise information available to us.
It also refactors code to emit the same "previous at" error message
for duplicate values in map literals.
Thanks to Emmanuel Odeke for the test case.
Fixes#33460.
Change-Id: Iec69542ccd4aad594dde8df02d1b880a422c5622
Reviewed-on: https://go-review.googlesource.com/c/go/+/188901
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Noticed while preparing a CL for Go 1.14 to remove esc.go.
Change-Id: Ic12be33f5b16c8424d85f373fa450247be086078
Reviewed-on: https://go-review.googlesource.com/c/go/+/173298
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
