This CL moves the load/store combining tests into asmcheck.
In addition at being more compact, it's also now easier to
spot what it is missing in each architecture.
While doing so, I think I uncovered a bug in ppc64le and arm64
rules, because they fail to load/store combine in non-trivial
functions. Not sure why, I'll open an issue.
Change-Id: Ia1572d53c0553d9104f3e52b95e4d1768a8440a3
Reviewed-on: https://go-review.googlesource.com/98441
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Before this change, in case of any failure, asmcheck was
dumping to stderr the whole output of compile -S, which
can be very long if it contains multiple functions.
Make it so it filters the output to only display the
assembly output of functions for which at least one opcode
check failed. This greatly simplifies debugging.
Change-Id: I1bbf54473b8252a3384e2c1dade82d926afc119d
Reviewed-on: https://go-review.googlesource.com/98444
Run-TryBot: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Keith Randall <khr@golang.org>
Currently, the top-level testsuite always uses whatever version
of Go is found in the PATH to execute all the tests. This
forces the developers to tweak the PATH to run the testsuite.
Change it to use the same version of Go used to run run.go.
This allows developers to run the testsuite using the tip
compiler by simply saying "../bin/go run run.go".
I think this is a better solution compared to always forcing
"../bin/go", because it allows developers to run the testsuite
using different Go versions, for instance to check if a new
test is fixed in tip compared to the installed compiler.
Fixes#24217
Change-Id: I41b299c753b6e77c41e28be9091b2b630efea9d2
Reviewed-on: https://go-review.googlesource.com/98439
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Use the __vdso_clock_gettime fast path via the vDSO on linux/arm to
speed up nanotime and walltime. This results in the following
performance improvement for time.Now on a RaspberryPi 3 (running
32bit Raspbian, i.e. GOOS=linux/GOARCH=arm):
name old time/op new time/op delta
TimeNow 0.99µs ± 0% 0.39µs ± 1% -60.74% (p=0.000 n=12+20)
Change-Id: I3598278a6c88d7f6a6ce66c56b9d25f9dd2f4c9a
Reviewed-on: https://go-review.googlesource.com/98095
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The previous type cache is quadratic in time in the situation where
new types are continually encountered. Now that it is possible to dynamically
create new types with the reflect package, this can cause json to
perform very poorly.
Switch to sync.Map which does well when the cache has hit steady state,
but also handles occasional updates in better than quadratic time.
benchmark old ns/op new ns/op delta
BenchmarkTypeFieldsCache/MissTypes1-8 14817 16202 +9.35%
BenchmarkTypeFieldsCache/MissTypes10-8 70926 69144 -2.51%
BenchmarkTypeFieldsCache/MissTypes100-8 976467 208973 -78.60%
BenchmarkTypeFieldsCache/MissTypes1000-8 79520162 1750371 -97.80%
BenchmarkTypeFieldsCache/MissTypes10000-8 6873625837 16847806 -99.75%
BenchmarkTypeFieldsCache/HitTypes1000-8 7.51 8.80 +17.18%
BenchmarkTypeFieldsCache/HitTypes10000-8 7.58 8.68 +14.51%
The old implementation takes 12 minutes just to build a cache of size 1e5
due to the quadratic behavior. I did not bother benchmark sizes above that.
Change-Id: I5e6facc1eb8e1b80e5ca285e4dd2cc8815618dad
Reviewed-on: https://go-review.googlesource.com/76850
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Make ReadSubKeyNames work even if key is opened with only
ENUMERATE_SUB_KEYs access rights mask.
Fixes#23869
Change-Id: I138bd51715fdbc3bda05607c64bde1150f4fe6b2
Reviewed-on: https://go-review.googlesource.com/97435
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Move the IndexByte function from the runtime to a new bytealg package.
The new package will eventually hold all the optimized assembly for
groveling through byte slices and strings. It seems a better home for
this code than randomly keeping it in runtime.
Once this is in, the next step is to move the other functions
(Compare, Equal, ...).
Update #19792
This change seems complicated enough that we might just declare
"not worth it" and abandon. Opinions welcome.
The core assembly is all unchanged, except minor modifications where
the code reads cpu feature bits.
The wrapper functions have been cleaned up as they are now actually
checked by vet.
Change-Id: I9fa75bee5d85db3a65b3fd3b7997e60367523796
Reviewed-on: https://go-review.googlesource.com/98016
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Since that method uses 'mux.m', we need to lock the mutex to avoid data races.
Change-Id: I998448a6e482b5d6a1b24f3354bb824906e23172
GitHub-Last-Rev: 163a7d4942
GitHub-Pull-Request: golang/go#23994
Reviewed-on: https://go-review.googlesource.com/96575
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TestEmptyDwarfRanges has been added in CL 94816.
This test is failing on Plan 9 because executables
don't have a DWARF symbol table.
Fixes#24226.
Change-Id: Iff7e34b8c2703a2f19ee8087a4d64d0bb98496cd
Reviewed-on: https://go-review.googlesource.com/98275
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Even though undocumented, the assumption is the Event's link field
points to the following event in the future. The new span/task event
processing breaks the assumption.
Change-Id: I4ce2f30c67c4f525ec0a121a7e43d8bdd2ec3f77
Reviewed-on: https://go-review.googlesource.com/96395
Reviewed-by: Heschi Kreinick <heschi@google.com>
When recursively calling walkexpr, r.Type is still the untyped value.
It then sometimes recursively calls finishcompare, which complains that
you can't compare the resulting expression to that untyped value.
Updates #23834.
Change-Id: I6b7acd3970ceaff8da9216bfa0ae24aca5dee828
Reviewed-on: https://go-review.googlesource.com/97856
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Add DWARF register mappings for ARM64, so that that arch will become
usable with "-dwarflocationlists". [NB: I've plugged in a set of
numbers from the doc, but this will require additional manual testing.]
Change-Id: Id9aa63857bc8b4f5c825f49274101cf372e9e856
Reviewed-on: https://go-review.googlesource.com/82515
Reviewed-by: Heschi Kreinick <heschi@google.com>
When generating location lists, batch up changes for all zero-width
instructions, not just phis. This prevents the creation of location list
entries that don't actually cover any instructions.
This isn't perfect because of the caveats in the prior CL (Copy is
zero-width sometimes) but in practice this seems to fix all of the empty
lists in std.
Change-Id: Ice4a9ade36b6b24ca111d1494c414eec96e5af25
Reviewed-on: https://go-review.googlesource.com/97958
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Add a bool to opInfo to indicate if an Op never results in any
instructions. This is a conservative approximation: some operations,
like Copy, may or may not generate code depending on their arguments.
I built the list by reading each arch's ssaGenValue function. Hopefully
I got them all.
Change-Id: I130b251b65f18208294e129bb7ddc3f91d57d31d
Reviewed-on: https://go-review.googlesource.com/97957
Reviewed-by: Keith Randall <khr@golang.org>
This reverts commit 7365fac2db.
Reason for revert: breaks the build on some architectures, reading unmapped pages?
Change-Id: I3a8c02dc0b649269faacea79ecd8213defa97c54
Reviewed-on: https://go-review.googlesource.com/97995
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LLVM tools, particularly lldb and dsymutil, don't support base address
selection entries in location lists. When targeting GOOS=darwin,
mode, have the linker translate location lists to CU-relative form
instead.
Technically, this isn't necessary when linking internally, as long as
nobody plans to use anything other than Delve to look at the DWARF. But
someone might want to use lldb, and it's really confusing when dwarfdump
shows gibberish for the location entries. The performance cost isn't
noticeable, so enable it even for internal linking.
Doing this in the linker is a little weird, but it was more expensive in
the compiler, probably because the compiler is much more stressful to
the GC. Also, if we decide to only do it for external linking, the
compiler can't see the link mode.
Benchmark before and after this commit on Mac with -dwarflocationlists=1:
name old time/op new time/op delta
StdCmd 21.3s ± 1% 21.3s ± 1% ~ (p=0.310 n=27+27)
Only StdCmd is relevant, because only StdCmd runs the linker. Whatever
the cost is here, it's not very large.
Change-Id: Ic8ef780d0e263230ce6aa3ca3a32fc9abd750b1e
Reviewed-on: https://go-review.googlesource.com/97956
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Some SSA values don't translate into any instructions. If a function
began with two of them, and both modified the storage of the same
variable, we'd end up with a location list entry that started and ended
at 0. That looks like an end-of-list entry, which would then confuse
downstream tools, particularly the fixup in the linker.
"Fix" this by changing the end of such entries to 1. Should be harmless,
since AFAIK we don't generate any 1-byte instructions. Later CLs will
reduce the frequency of these entries anyway.
Change-Id: I9b7e5e69f914244cc826fb9f4a6acfe2dc695f81
Reviewed-on: https://go-review.googlesource.com/97955
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
In RET instruction, the operand is the return jump's target,
which should be put in Prog.To.
Add an action "buildrundir" to the test driver, which builds
(compile+assemble+link) the code in a directory and runs the
resulting binary.
Fixes#23838.
Change-Id: I7ebe7eda49024b40a69a24857322c5ca9c67babb
Reviewed-on: https://go-review.googlesource.com/94175
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
bytes.IndexByte is heavily optimized.
Use it in findnull.
name old time/op new time/op delta
GoString-8 65.5ns ± 1% 40.2ns ± 1% -38.62% (p=0.000 n=19+19)
findnull is also used in gostringnocopy,
which is used in many hot spots in the runtime.
Fixes#23830
Change-Id: I2e6cb279c7d8078f8844065de684cc3567fe89d7
Reviewed-on: https://go-review.googlesource.com/97523
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Replace BYTE.. encodings with asm. This is possible due to asm
implementing more instructions and removal of
MOV $0, reg -> XOR reg, reg transformation from asm.
Change-Id: I011749ab6b3f64403ab6e746f3760c5841548b57
Reviewed-on: https://go-review.googlesource.com/97936
Run-TryBot: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
useSSE41 was used inside asm implementation of floor to select between base and ss4 code path.
We intrinsified floor and left asm functions as a backup for non-sse4 systems.
This made variable unused, so remove it.
Change-Id: Ia2633de7c7cb1ef1d5b15a2366b523e481b722d9
Reviewed-on: https://go-review.googlesource.com/97935
Run-TryBot: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Change-Id: I030baaa0a0abf1e43449faaf676d389a28a868a3
Reviewed-on: https://go-review.googlesource.com/97857
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
This avoid simple bugs like "ADD" matching "FADD". Obviously
"ADD" will still match "ADDQ" so some care is still required
in this regard, but at least a first class of possible errors
is taken care of.
Change-Id: I7deb04c31de30bedac9c026d9889ace4a1d2adcb
Reviewed-on: https://go-review.googlesource.com/97817
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Keith Randall <khr@golang.org>
asmcheck comments now support a compact form of specifying
multiple checks for each platform, using the following syntax:
amd64:"SHL\t[$]4","SHR\t[$]4"
Negative checks are also parsed using the following syntax:
amd64:-"ROR"
though they are still not working.
Moreover, out-of-line comments have been implemented. This
allows to specify asmchecks on comment-only lines, that will
be matched on the first subsequent non-comment non-empty line.
// amd64:"XOR"
// arm:"EOR"
x ^= 1
Change-Id: I110c7462fc6a5c70fd4af0d42f516016ae7f2760
Reviewed-on: https://go-review.googlesource.com/97816
Reviewed-by: Keith Randall <khr@golang.org>
When the slice/string length is very large,
probably artifically large as in CL 97523,
adding BX (length) to R11 (pointer) overflows.
As a result, checking DI < R11 yields the wrong result.
Since they will be equal when the loop is done,
just check DI != R11 instead.
Yes, the pointer itself could overflow, but if that happens,
something else has gone pretty wrong; not our concern here.
Fixes#24187
Change-Id: I2f60fc6ccae739345d01bc80528560726ad4f8c6
Reviewed-on: https://go-review.googlesource.com/97802
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Don’t panic if a subtest inadvertently calls FailNow
on a parent’s T. Instead, report the offending subtest
while still reporting the error with the ancestor test and
keep exiting goroutines.
Note that this implementation has a race if parallel
subtests are failing the parent concurrently.
This is fine:
Calling FailNow on a parent is considered an error
in principle, at the moment, and is reported if it is
detected. Having the race allows the race detector
to detect the error as well.
Fixes#22882
Change-Id: Ifa6d5e55bb88f6bcbb562fc8c99f1f77e320015a
Reviewed-on: https://go-review.googlesource.com/97635
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
Reviewed-by: Kunpei Sakai <namusyaka@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The top-level test harness is modified to support a new kind
of test: "asmcheck". This is meant to replace asm_test.go
as an easier and more readable way to test code generation.
I've added a couple of codegen tests to get initial feedback
on the syntax. I've created them under a common "codegen"
subdirectory, so that it's easier to run them all with
"go run run.go -v codegen".
The asmcheck syntax allows to insert line comments that
can specify a regular expression to match in the assembly code,
for multiple architectures (the testsuite will automatically
build each testfile multiple times, one per mentioned architecture).
Negative matches are unsupported for now, so this cannot fully
replace asm_test yet.
Change-Id: Ifdbba389f01d55e63e73c99e5f5449e642101d55
Reviewed-on: https://go-review.googlesource.com/97355
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Alberto Donizetti <alb.donizetti@gmail.com>
All functions defined in syscall2_solaris.go have the respective libc_*
var in syscall_solaris.go, except for libc_close. Move it from
os3_solaris.go
Remove unused libc_fstat.
Order go:cgo_import_dynamic and go:linkname lists in
syscall2_solaris.go alphabetically.
Change-Id: I9f12fa473cf1ae351448ac45597c82a67d799c31
Reviewed-on: https://go-review.googlesource.com/97736
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Consider the following:
type child struct{ Field string }
type parent struct{ child }
p := new(parent)
v := reflect.ValueOf(p).Elem().Field(0)
v.Field(0).SetString("hello") // v.Field = "hello"
v = v.Addr().Elem() // v = *(&v)
v.Field(0).SetString("goodbye") // v.Field = "goodbye"
It would appear that v.Addr().Elem() should have the same value, and
that it would be safe to set "goodbye".
However, after CL 66331, any interspersed calls between Field calls
causes the RO flag to be set.
Thus, setting to "goodbye" actually causes a panic.
That CL affects decodeState.indirect which assumes that back-to-back
Value.Addr().Elem() is side-effect free. We fix that logic to keep
track of the Addr() and Elem() calls and set v back to the original
after a full round-trip has occured.
Fixes#24152
Updates #24153
Change-Id: Ie50f8fe963f00cef8515d89d1d5cbc43b76d9f9c
Reviewed-on: https://go-review.googlesource.com/97796
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Instructions LDARB, LDARH, LDAXPW, LDAXP, STLRB, STLRH, STLXP, STLXPW, STXP,
STXPW have been added before, but they are not enabled. This CL enabled them.
Change the form of LDXP and LDXPW to the form of LDP, and fix a bug of STLXP.
Change-Id: I5d2b51494b92451bf6b072c65cfdd8acf07e9b54
Reviewed-on: https://go-review.googlesource.com/96215
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
