This is one of several changes that were part of a larger rewrite
which I made in early 2019 after switching to the new number literal
syntax implementation. The purpose of the rewrite was to simplify
reading of source code (Unicode character by character) and speed up
the scanner but was never submitted for review due to other priorities.
Part 2 of 3:
This change contains improvements to the scanner error messages:
- Use "rune literal" rather than "character literal" to match the
spec nomenclature.
- Shorter, more to the point error messages.
(For instance, "more than one character in rune literal" rather
than "invalid character literal (more than one character)", etc.)
Change-Id: I1aaf79003374a68dbb05926437ed305cf2a8ec96
Reviewed-on: https://go-review.googlesource.com/c/go/+/221602
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
There are still two places in src/runtime/string.go that use
staticbytes, so we cannot delete it just yet.
There is a new codegen test to verify that the index calculation
is constant-folded, at least on amd64. ppc64, mips[64] and s390x
cannot currently do that.
There is also a new runtime benchmark to ensure that this does not
slow down performance (tested against parent commit):
name old time/op new time/op delta
ConvT2EByteSized/bool-4 1.07ns ± 1% 1.07ns ± 1% ~ (p=0.060 n=14+15)
ConvT2EByteSized/uint8-4 1.06ns ± 1% 1.07ns ± 1% ~ (p=0.095 n=14+15)
Updates #37612
Change-Id: I5ec30738edaa48cda78dfab4a78e24a32fa7fd6a
Reviewed-on: https://go-review.googlesource.com/c/go/+/221957
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Trying this CL again, with a fixed test that allows platforms
to disagree on the exact behavior of converting NaNs.
We store 32-bit floating point constants in a 64-bit field, by
converting that 32-bit float to 64-bit float to store it, and convert
it back to use it.
That works for *almost* all floating-point constants. The exception is
signaling NaNs. The round trip described above means we can't represent
a 32-bit signaling NaN, because conversions strip the signaling bit.
To fix this issue, just forbid NaNs as floating-point constants in SSA
form. This shouldn't affect any real-world code, as people seldom
constant-propagate NaNs (except in test code).
Additionally, NaNs are somewhat underspecified (which of the many NaNs
do you get when dividing 0/0?), so when cross-compiling there's a
danger of using the compiler machine's NaN regime for some math, and
the target machine's NaN regime for other math. Better to use the
target machine's NaN regime always.
Update #36400
Change-Id: Idf203b688a15abceabbd66ba290d4e9f63619ecb
Reviewed-on: https://go-review.googlesource.com/c/go/+/221790
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Print the bytes of the instruction that generated a SIGILL.
This should help us respond to bug reports without having to
go back-and-forth with the reporter to get the instruction involved.
Might also help with SIGILL problems that are difficult to reproduce.
Update #37513
Change-Id: I33059b1dbfc97bce16142a843f32a88a6547e280
Reviewed-on: https://go-review.googlesource.com/c/go/+/221431
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Open-coded defers were fixed and re-enabled on riscv64, however this test was
inadvertantly left disabled.
Updates #36786
Change-Id: I128fc84baa3d51f50d173e19e52051dc4d9a07c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/220920
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The existing implementation outputs inline cost iff function cannot be inlined with Debug['m'] > 1, the cost info is also useful if the function is inlineable.
Fixes#36780
Change-Id: Ic96f6baf96aee25fb4b33d31d4d644dc2310e536
Reviewed-on: https://go-review.googlesource.com/c/go/+/216778
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
The generic Greater and Geq ops can always be replaced with the Less and
Leq ops. This CL therefore removes them. This simplifies the compiler since
it reduces the number of operations that need handling in both code and in
rewrite rules. This will be especially true when adding control flow
optimizations such as the integer-in-range optimizations in CL 165998.
Change-Id: If0648b2b19998ac1bddccbf251283f3be4ec3040
Reviewed-on: https://go-review.googlesource.com/c/go/+/220417
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Ensure that any comparison between two values has the same argument
order. This helps ensure that they can be eliminated during the
lowered CSE pass which will be particularly important if we eliminate
the Greater and Geq ops (see #37316).
Example:
CMP R0, R1
BLT L1
CMP R1, R0 // different order, cannot eliminate
BEQ L2
CMP R0, R1
BLT L1
CMP R0, R1 // same order, can eliminate
BEQ L2
This does have some drawbacks. Notably comparisons might 'flip'
direction in the assembly output after even small changes to the
code or compiler. It should help make optimizations more reliable
however.
compilecmp master -> HEAD
master (218f4572f5): text/template: make reflect.Value indirections more robust
HEAD (f1661fef3e): cmd/compile: canonicalize comparison argument order
platform: linux/amd64
file before after Δ %
api 6063927 6068023 +4096 +0.068%
asm 5191757 5183565 -8192 -0.158%
cgo 4893518 4901710 +8192 +0.167%
cover 5330345 5326249 -4096 -0.077%
fix 3417778 3421874 +4096 +0.120%
pprof 14889456 14885360 -4096 -0.028%
test2json 2848138 2844042 -4096 -0.144%
trace 11746239 11733951 -12288 -0.105%
total 132739173 132722789 -16384 -0.012%
Change-Id: I11736b3fe2a4553f6fc65018f475e88217fa22f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/220425
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This reverts CL 213477.
Reason for revert: tests are failing on linux-mips*-rtrk builders.
Change-Id: I8168f7450890233f1bd7e53930b73693c26d4dc0
Reviewed-on: https://go-review.googlesource.com/c/go/+/220897
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We store 32-bit floating point constants in a 64-bit field, by
converting that 32-bit float to 64-bit float to store it, and convert
it back to use it.
That works for *almost* all floating-point constants. The exception is
signaling NaNs. The round trip described above means we can't represent
a 32-bit signaling NaN, because conversions strip the signaling bit.
To fix this issue, just forbid NaNs as floating-point constants in SSA
form. This shouldn't affect any real-world code, as people seldom
constant-propagate NaNs (except in test code).
Additionally, NaNs are somewhat underspecified (which of the many NaNs
do you get when dividing 0/0?), so when cross-compiling there's a
danger of using the compiler machine's NaN regime for some math, and
the target machine's NaN regime for other math. Better to use the
target machine's NaN regime always.
This has been a bug since 1.10, and there's an easy workaround
(declare a global varaible containing the signaling NaN pattern, and
use that as the argument to math.Float32frombits) so we'll fix it in
1.15.
Fixes#36400
Update #36399
Change-Id: Icf155e743281560eda2eed953d19a829552ccfda
Reviewed-on: https://go-review.googlesource.com/c/go/+/213477
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
The address calculations in the example end up doing x << 4 + y + 0.
Before this CL we use a SHLQ+LEAQ. Since the constant offset is 0,
we can use SHLQ+ADDQ instead.
Change-Id: Ia048c4fdbb3a42121c7e1ab707961062e8247fca
Reviewed-on: https://go-review.googlesource.com/c/go/+/209959
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
The wrapper takes a pointer to the argument, not the argument itself.
Fixes#36705
Change-Id: I566d4457d00bf5b84e4a8315a26516975f0d7e10
Reviewed-on: https://go-review.googlesource.com/c/go/+/215942
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
We should panic in this situation. Rewriting to a SSA op just leads
to a compiler panic.
Fixes#36259
Change-Id: I6e0bccbed7dd0fdac7ebae76b98a211947947386
Reviewed-on: https://go-review.googlesource.com/c/go/+/212405
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
• Inline check function because it's more readable.
• Delete toolPath because it was unused.
• Use strings.TrimPrefix because it's simpler.
• Remove out variable because its value was unused.
• Rename serr to err because it's more consistent.
Change-Id: I084fb4f8b399578834d5eea29a673c386cf3a357
Reviewed-on: https://go-review.googlesource.com/c/go/+/218701
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
The error message is now positioned at the statement position (which is
an identifing token, such as the '=' for assignments); and in case of
assignments it emphasizes the assignment by putting the Lhs and Rhs
in parentheses. Finally, the wording is changed from "use of * as value"
to the stronger "cannot use * as value" (for which there is precedent
elsewhere in the parser).
Fixes#36858.
Change-Id: Ic3f101bba50f58e3a1d9b29645066634631f2d61
Reviewed-on: https://go-review.googlesource.com/c/go/+/218337
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Brad's battery died on a plane and the file stayed open for 8 years
without anyone noticing. 😄
Someone noticed in https://github.com/gopherjs/gopherjs/pull/950.
Updates #2833
Change-Id: I46b28ac014a8c355be94e026615f119f96e5d51a
Reviewed-on: https://go-review.googlesource.com/c/go/+/218700
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
The R_CALLRISCV relocation marker is on the JALR instruction, however the actual
relocation is currently two instructions previous for the AUIPC+ADDI sequence.
Adjust the platform dependent offset accordingly and re-enable open-coded defers.
Fixes#36786.
Change-Id: I71597c193c447930fbe94ce44b7355e89ae877bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/216797
Run-TryBot: Joel Sing <joel@sing.id.au>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This test expects that open-coded defers are enabled, which is not currently
the case on riscv64.
Updates issue #27532 and #36786.
Change-Id: I94bb558c5b0734b4cfe5ae12873be81026009bcf
Reviewed-on: https://go-review.googlesource.com/c/go/+/216777
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Open-coded defers are currently broken on riscv64 - disable them for the
time being. All of the standard package tests now pass on linux/riscv64.
Updates issue #27532 and #36786
Change-Id: I20fc25ce91dfad48be32409ba5c64ca9a6acef1d
Reviewed-on: https://go-review.googlesource.com/c/go/+/216517
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
On PPC64, MOVWload, MOVDload, and MOVDstore are assembled to a
"DS from" instruction which requiers the offset is a multiple of
4. Only fold offset to such instructions if it is a multiple of 4.
Fixes#36723.
"GOARCH=ppc64 GOOS=linux go build -gcflags=all=-d=ssa/check/on std cmd"
passes now.
Change-Id: I67f2a6ac02f0d33d470f68ff54936c289a4c765b
Reviewed-on: https://go-review.googlesource.com/c/go/+/216379
Reviewed-by: Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
This disables some tests that are unsupported on riscv64 and adds support
for risc64 to test/nosplit.
Updates #27532, #36739 and #36765
Change-Id: I0a57797a05bc80236709fc240c0a0efb0ee0d16b
Reviewed-on: https://go-review.googlesource.com/c/go/+/216263
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
CL 214679 added a -race test which shouldn't be run when cgo is not
enabled.
Fixes the nocgo builder.
Change-Id: Iceddf802c4ef6c0de2c3a968e86342303d2d27d8
Reviewed-on: https://go-review.googlesource.com/c/go/+/215477
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This avoids the security problem in #29312 where two very deep, but
distinct, types are given the same name. They both make it to the
linker which chooses one, and the use of the other is now type unsafe.
Instead, give every very deep type its own name. This errs on the
other side, in that very deep types that should be convertible to each
other might now not be. But at least that's not a security hole.
Update #29312.
Change-Id: Iac0ebe73fdc50594fd6fbf7432eef65f9a053126
Reviewed-on: https://go-review.googlesource.com/c/go/+/213517
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
The gccgo compiler did not generate type descriptor for a pointer
to a type alias defined in another package, causing linking error.
The fix is CL 210787. This CL adds a test.
Updates #36085.
Change-Id: I3237c7fedb4d92fb2dc610ee2b88087f96dc2a1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/210858
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This test was recently added in CL 209961.
Apparently Windows can't seek a directory filehandle?
And move the test from test/fixedbugs (which is mostly for compiler bugs) to
an os package test.
Updates #36019
Change-Id: I626b69b0294471014901d0ccfeefe5e2c7651788
Reviewed-on: https://go-review.googlesource.com/c/go/+/210283
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
The first Readdirnames calls opendir and caches the result.
The behavior of that cached opendir result isn't specified on a seek
of the underlying fd. Free the opendir result on a seek so that
we'll allocate a new one the next time around.
Also fix wasm behavior in this regard, so that a seek to the
file start resets the Readdirnames position, regardless of platform.
p.s. I hate the Readdirnames API.
Fixes#35767.
Change-Id: Ieffb61b3c5cdd42591f69ab13f932003966f2297
Reviewed-on: https://go-review.googlesource.com/c/go/+/209961
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The fix for #35652 did not guarantee that it was using a non-empty
src position to replace an empty one. The new code checks again
and falls back to a more certain position. (The input in question
compiles to a single empty infinite loop, and none of the actual instructions
had any source position at all. That is a bug, but given the pathology
of this input, not one worth dealing with this late in the release cycle,
if ever.)
Literally:
00000 (5) TEXT "".f(SB), ABIInternal
00001 (5) PCDATA $0, $-2
00002 (5) PCDATA $1, $-2
00003 (5) FUNCDATA $0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
00004 (5) FUNCDATA $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
00005 (5) FUNCDATA $2, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
b2
00006 (?) XCHGL AX, AX
b6
00007 (+1048575) JMP 6
00008 (?) END
TODO: Add runtime.InfiniteLoop(), replace infinite loops with a call to
that, and use an eco-friendly runtime.gopark instead. (This was Cherry's
excellent idea.)
Updates #35652Fixes#35695
Change-Id: I4b9a841142ee4df0f6b10863cfa0721a7e13b437
Reviewed-on: https://go-review.googlesource.com/c/go/+/207964
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
The old recipe for making an infinite loop not be infinite
in the debugger could create an instruction (Prog) with a
line number not tied to any file (index == 0). This caused
downstream failures in DWARF processing.
So don't do that. Also adds a test, also adds a check+panic
to ensure that the next time this happens the error is less
mystifying.
Fixes#35652
Change-Id: I04f30bc94fdc4aef20dd9130561303ff84fd945e
Reviewed-on: https://go-review.googlesource.com/c/go/+/207613
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This reverts CL 207477, restoring CL 207352 with a fix for the
regression observed in the Windows builders.
cmd/compile evidently does not fully support NUL as an output on
Windows, so this time we write ignored 'compile' outputs
to temporary files (instead of os.DevNull as in CL 207352).
Updates #28387Fixes#35619
Change-Id: I2edc5727c3738fa1bccb4b74e50d114cf2a7fcff
Reviewed-on: https://go-review.googlesource.com/c/go/+/207602
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This allows maphash.Hash to be allocated on the stack for typical uses.
Fixes#35636
Change-Id: I8366507d26ea717f47a9fb46d3bd69ba799845ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/207444
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This CL detects infinite loops due to negative dereference cycles
during escape analysis, and terminates the loop gracefully. We still
fail to print a complete explanation of the escape path, but esc.go
didn't print *any* explanation for these test cases, so the release
blocking issue here is simply that we don't infinite loop.
Updates #35518.
Change-Id: I39beed036e5a685706248852f1fa619af3b7abbc
Reviewed-on: https://go-review.googlesource.com/c/go/+/206619
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Unlike function calls, when processing instructions that directly
fault we must not subtract 1 from the pc before looking up the
file/line information.
Since the file/line lookup unconditionally subtracts 1, add 1 to
the faulting instruction PCs to compensate.
Fixes#34123
Change-Id: Ie7361e3d2f84a0d4f48d97e5a9e74f6291ba7a8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/196962
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This API was added for #25819, where it was discussed as math.FMA.
The commit adding it used math.Fma, presumably for consistency
with the rest of the unusual names in package math
(Sincos, Acosh, Erfcinv, Float32bits, etc).
I believe that using an idiomatic Go name is more important here
than consistency with these other names, most of which are historical
baggage from C's standard library.
Early additions like Float32frombits happened before "uppercase for export"
(so they were originally like "float32frombits") and they were not properly
reconsidered when we uppercased the symbols to export them.
That's a mistake we live with.
The names of functions we have added since then, and even a few
that were legacy, are more properly Go-cased, such as IsNaN, IsInf,
and RoundToEven, rather than Isnan, Isinf, and Roundtoeven.
And also constants like MaxFloat32.
For new API, we should keep using proper Go-cased symbols
instead of minimally-upper-cased-C symbols.
So math.FMA, not math.Fma.
This API has not yet been released, so this change does not break
the compatibility promise.
This CL also modifies cmd/compile, since the compiler knows
the name of the function. I could have stopped at changing the
string constants, but it seemed to make more sense to use a
consistent casing everywhere.
Change-Id: I0f6f3407f41e99bfa8239467345c33945088896e
Reviewed-on: https://go-review.googlesource.com/c/go/+/205317
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We seem to lack any tests for some corner cases of itab.init
(multiple methods with the same name, breaking itab.init doesn't
seem to fail any tests). We also lack tests that fix text of panics.
Add more tests for itab.init.
Change-Id: Id6b536179ba6b0d45c3cb9dc1c66b9311d0ab85e
Reviewed-on: https://go-review.googlesource.com/c/go/+/202451
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In the dev.link branch we implemented the new object file format
and (part of) the linker improvements described in
https://golang.org/s/better-linker
The new object file is index-based and provides random access.
The linker maps the object files into read-only memory, and
access symbols on-demand using indices, as opposed to reading
all object files sequentially into the heap with the old format.
The linker carries symbol informations using indices (as opposed
to Symbol data structure). Symbols are created after the
reachability analysis, and only created for reachable symbols.
This reduces the linker's memory usage.
Linking cmd/compile, it creates ~25% fewer Symbols, and reduces
memory usage (inuse_space) by ~15%. (More results from Than.)
Currently, both the old and new object file formats are supported.
The old format is used by default. The new format can be turned
on by using the compiler/assembler/linker's -newobj flag. Note
that the flag needs to be specified consistently to all
compilations, i.e.
go build -gcflags=all=-newobj -asmflags=all=-newobj -ldflags=-newobj
Change-Id: Ia0e35306b5b9b5b19fdc7fa7c602d4ce36fa6abd
The logic for keeping arguments alive for calls to //go:uintptrescapes
functions was only applying to direct function calls. This CL changes
it to also apply to direct method calls, which should address most
uses of Proc.Call and LazyProc.Call.
It's still an open question (#34684) whether other call forms (e.g.,
method expressions, or indirect calls via function values, method
values, or interfaces).
Fixes#34474.
Change-Id: I874f97145972b0e237a4c9e8926156298f4d6ce0
Reviewed-on: https://go-review.googlesource.com/c/go/+/198043
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
CL 200958 adds skipping empty init function feature without any tests
for it. A codegen test sounds ideal, but it's unlikely that we can make
one for now, so use a program to manipulate runtime/proc.go:initTask
directly.
Updates #34869
Change-Id: I2683b9a1ace36af6861af02a3a9fb18b3110b282
Reviewed-on: https://go-review.googlesource.com/c/go/+/204217
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
When we do a successful recover of a panic, we resume normal execution by
returning from the frame that had the deferred call that did the recover (after
executing any remaining deferred calls in that frame).
However, suppose we have called runtime.Goexit and there is a panic during one of the
deferred calls run by the Goexit. Further assume that there is a deferred call in
the frame of the Goexit or a parent frame that does a recover. Then the recovery
process will actually resume normal execution above the Goexit frame and hence
abort the Goexit. We will not terminate the thread as expected, but continue
running in the frame above the Goexit.
To fix this, we explicitly create a _panic object for a Goexit call. We then
change the "abort" behavior for Goexits, but not panics. After a recovery, if the
top-level panic is actually a Goexit that is marked to be aborted, then we return
to the Goexit defer-processing loop, so that the Goexit is not actually aborted.
Actual code changes are just panic.go, runtime2.go, and funcid.go. Adjusted the
test related to the new Goexit behavior (TestRecoverBeforePanicAfterGoexit) and
added several new tests of aborted panics (whose behavior has not changed).
Fixes#29226
Change-Id: Ib13cb0074f5acc2567a28db7ca6912cfc47eecb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/200081
Run-TryBot: Dan Scales <danscales@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
"declared and not used" is technically correct, but might confuse
the user. Switching "and" to "but" will hopefully create the
contrast for the users: they did one thing (declaration), but
not the other --- actually using the variable.
This new message is still not ideal (specifically, declared is not
entirely precise here), but at least it matches the other parsers
and is one step in the right direction.
Change-Id: I725c7c663535f9ab9725c4b0bf35b4fa74b0eb20
Reviewed-on: https://go-review.googlesource.com/c/go/+/203282
Run-TryBot: Minux Ma <minux@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Updates #35157 (the bug there was fixed by CL200861)
Change-Id: I67069207b4cdc2ad4a475dd0bbc8555ecc5f534f
Reviewed-on: https://go-review.googlesource.com/c/go/+/203598
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alberto Donizetti <alb.donizetti@gmail.com>
Generate inline code at defer time to save the args of defer calls to unique
(autotmp) stack slots, and generate inline code at exit time to check which defer
calls were made and make the associated function/method/interface calls. We
remember that a particular defer statement was reached by storing in the deferBits
variable (always stored on the stack). At exit time, we check the bits of the
deferBits variable to determine which defer function calls to make (in reverse
order). These low-cost defers are only used for functions where no defers
appear in loops. In addition, we don't do these low-cost defers if there are too
many defer statements or too many exits in a function (to limit code increase).
When a function uses open-coded defers, we produce extra
FUNCDATA_OpenCodedDeferInfo information that specifies the number of defers, and
for each defer, the stack slots where the closure and associated args have been
stored. The funcdata also includes the location of the deferBits variable.
Therefore, for panics, we can use this funcdata to determine exactly which defers
are active, and call the appropriate functions/methods/closures with the correct
arguments for each active defer.
In order to unwind the stack correctly after a recover(), we need to add an extra
code segment to functions with open-coded defers that simply calls deferreturn()
and returns. This segment is not reachable by the normal function, but is returned
to by the runtime during recovery. We set the liveness information of this
deferreturn() to be the same as the liveness at the first function call during the
last defer exit code (so all return values and all stack slots needed by the defer
calls will be live).
I needed to increase the stackguard constant from 880 to 896, because of a small
amount of new code in deferreturn().
The -N flag disables open-coded defers. '-d defer' prints out the kind of defer
being used at each defer statement (heap-allocated, stack-allocated, or
open-coded).
Cost of defer statement [ go test -run NONE -bench BenchmarkDefer$ runtime ]
With normal (stack-allocated) defers only: 35.4 ns/op
With open-coded defers: 5.6 ns/op
Cost of function call alone (remove defer keyword): 4.4 ns/op
Text size increase (including funcdata) for go binary without/with open-coded defers: 0.09%
The average size increase (including funcdata) for only the functions that use
open-coded defers is 1.1%.
The cost of a panic followed by a recover got noticeably slower, since panic
processing now requires a scan of the stack for open-coded defer frames. This scan
is required, even if no frames are using open-coded defers:
Cost of panic and recover [ go test -run NONE -bench BenchmarkPanicRecover runtime ]
Without open-coded defers: 62.0 ns/op
With open-coded defers: 255 ns/op
A CGO Go-to-C-to-Go benchmark got noticeably faster because of open-coded defers:
CGO Go-to-C-to-Go benchmark [cd misc/cgo/test; go test -run NONE -bench BenchmarkCGoCallback ]
Without open-coded defers: 443 ns/op
With open-coded defers: 347 ns/op
Updates #14939 (defer performance)
Updates #34481 (design doc)
Change-Id: I63b1a60d1ebf28126f55ee9fd7ecffe9cb23d1ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/202340
Reviewed-by: Austin Clements <austin@google.com>
This change introduces an arm intrinsic that generates the FMULAD
instruction for the fused-multiply-add operation on systems that
support it. System support is detected via cpu.ARM.HasVFPv4. A rewrite
rule translates the generic intrinsic to FMULAD.
Updates #25819.
Change-Id: I8459e5dd1cdbdca35f88a78dbeb7d387f1e20efa
Reviewed-on: https://go-review.googlesource.com/c/go/+/142117
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
To permit ssa-level optimization, this change introduces an amd64 intrinsic
that generates the VFMADD231SD instruction for the fused-multiply-add
operation on systems that support it. System support is detected via
cpu.X86.HasFMA. A rewrite rule can then translate the generic ssa intrinsic
("Fma") to VFMADD231SD.
The benchmark compares the software implementation (old) with the intrinsic
(new).
name old time/op new time/op delta
Fma-4 27.2ns ± 1% 1.0ns ± 9% -96.48% (p=0.008 n=5+5)
Updates #25819.
Change-Id: I966655e5f96817a5d06dff5942418a3915b09584
Reviewed-on: https://go-review.googlesource.com/c/go/+/137156
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
In order to make math.FMA a compiler intrinsic for ISAs like ARM64,
PPC64[le], and S390X, a generic 3-argument opcode "Fma" is provided and
rewritten as
ARM64: (Fma x y z) -> (FMADDD z x y)
PPC64: (Fma x y z) -> (FMADD x y z)
S390X: (Fma x y z) -> (FMADD z x y)
Updates #25819.
Change-Id: Ie5bc628311e6feeb28ddf9adaa6e702c8c291efa
Reviewed-on: https://go-review.googlesource.com/c/go/+/131959
Run-TryBot: Akhil Indurti <aindurti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
We need to explicitly convert pointers to unsafe.Pointer before
passing to the runtime checkptr instrumentation in case the user
declared their own type with underlying type unsafe.Pointer.
Updates #22218.
Fixes#34966.
Change-Id: I3baa2809d77f8257167cd78f57156f819130baa8
Reviewed-on: https://go-review.googlesource.com/c/go/+/201782
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Generate inline code at defer time to save the args of defer calls to unique
(autotmp) stack slots, and generate inline code at exit time to check which defer
calls were made and make the associated function/method/interface calls. We
remember that a particular defer statement was reached by storing in the deferBits
variable (always stored on the stack). At exit time, we check the bits of the
deferBits variable to determine which defer function calls to make (in reverse
order). These low-cost defers are only used for functions where no defers
appear in loops. In addition, we don't do these low-cost defers if there are too
many defer statements or too many exits in a function (to limit code increase).
When a function uses open-coded defers, we produce extra
FUNCDATA_OpenCodedDeferInfo information that specifies the number of defers, and
for each defer, the stack slots where the closure and associated args have been
stored. The funcdata also includes the location of the deferBits variable.
Therefore, for panics, we can use this funcdata to determine exactly which defers
are active, and call the appropriate functions/methods/closures with the correct
arguments for each active defer.
In order to unwind the stack correctly after a recover(), we need to add an extra
code segment to functions with open-coded defers that simply calls deferreturn()
and returns. This segment is not reachable by the normal function, but is returned
to by the runtime during recovery. We set the liveness information of this
deferreturn() to be the same as the liveness at the first function call during the
last defer exit code (so all return values and all stack slots needed by the defer
calls will be live).
I needed to increase the stackguard constant from 880 to 896, because of a small
amount of new code in deferreturn().
The -N flag disables open-coded defers. '-d defer' prints out the kind of defer
being used at each defer statement (heap-allocated, stack-allocated, or
open-coded).
Cost of defer statement [ go test -run NONE -bench BenchmarkDefer$ runtime ]
With normal (stack-allocated) defers only: 35.4 ns/op
With open-coded defers: 5.6 ns/op
Cost of function call alone (remove defer keyword): 4.4 ns/op
Text size increase (including funcdata) for go cmd without/with open-coded defers: 0.09%
The average size increase (including funcdata) for only the functions that use
open-coded defers is 1.1%.
The cost of a panic followed by a recover got noticeably slower, since panic
processing now requires a scan of the stack for open-coded defer frames. This scan
is required, even if no frames are using open-coded defers:
Cost of panic and recover [ go test -run NONE -bench BenchmarkPanicRecover runtime ]
Without open-coded defers: 62.0 ns/op
With open-coded defers: 255 ns/op
A CGO Go-to-C-to-Go benchmark got noticeably faster because of open-coded defers:
CGO Go-to-C-to-Go benchmark [cd misc/cgo/test; go test -run NONE -bench BenchmarkCGoCallback ]
Without open-coded defers: 443 ns/op
With open-coded defers: 347 ns/op
Updates #14939 (defer performance)
Updates #34481 (design doc)
Change-Id: I51a389860b9676cfa1b84722f5fb84d3c4ee9e28
Reviewed-on: https://go-review.googlesource.com/c/go/+/190098
Reviewed-by: Austin Clements <austin@google.com>
Switch the default to new object files.
Internal linking cgo is disabled for now, as it does not work yet
in newobj mode.
Shared libraries are also broken.
Disable some tests that are known broken for now.
Change-Id: I8ca74793423861d607a2aa7b0d89a4f4d4ca7671
Reviewed-on: https://go-review.googlesource.com/c/go/+/200161
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
The Go spec requires
If a deferred function value evaluates to nil, execution
panics when the function is invoked, not when the "defer"
statement is executed.
On Wasm and AIX, currently we actually emit a nil check at the
point of defer statement, which will make it panic too early.
This CL fixes this.
Also, on Wasm, now the nil function will be passed through
deferreturn to jmpdefer, which does an explicit nil check and
calls sigpanic if it is nil. This sigpanic, being called from
assembly, is ABI0. So change the assembler backend to also
handle sigpanic in ABI0.
Fixes#34926.
Updates #8047.
Change-Id: I28489a571cee36d2aef041f917b8cfdc31d557d4
Reviewed-on: https://go-review.googlesource.com/c/go/+/201297
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When a subsequent load/store of a ptr makes the nil check of that pointer
unnecessary, if their lines differ, change the line of the load/store
to that of the nilcheck, and attempt to rehome the load/store position
instead.
This fix makes profiling less accurate in order to make panics more
informative.
Fixes#33724
Change-Id: Ib9afaac12fe0d0320aea1bf493617facc34034b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/200197
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Add to the testcase originally created for issue 34577 so
as to also trigger the error condition for issue 34852 (the
two bugs are closely related).
Updates #34577.
Updates #34852.
Change-Id: I2347369652ce500184347606b2bb3e76d802b204
Reviewed-on: https://go-review.googlesource.com/c/go/+/201017
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When assessing whether A <= B, the poset's OrderedOrEqual has a passing
condition which permits A <= B, but is not sufficient to infer that A <= B.
This CL removes that incorrect passing condition.
Having identified that A and B are in the poset, the method will report that
A <= B if any of these three conditions are true:
(1) A and B are the same node in the poset.
- This means we know that A == B.
(2) There is a directed path, strict or not, from A -> B
- This means we know that, at least, A <= B, but A < B is possible.
(3) There is a directed path from B -> A, AND that path has no strict edges.
- This means we know that B <= A, but do not know that B < A.
In condition (3), we do not have enough information to say that A <= B, rather
we only know that B == A (which satisfies A <= B) is possible. The way I
understand it, a strict edge shows a known, strictly-ordered relation (<) but
the lack of a strict edge does not show the lack of a strictly-ordered relation.
The difference is highlighted by the example in #34802, where a bounds check is
incorrectly removed by prove, such that negative indexes into a slice
succeed:
n := make([]int, 1)
for i := -1; i <= 0; i++ {
fmt.Printf("i is %d\n", i)
n[i] = 1 // No Bounds check, program runs, assignment to n[-1] succeeds!!
}
When prove is checking the negative/failed branch from the bounds check at n[i],
in the signed domain we learn (0 > i || i >= len(n)). Because prove can't learn
the OR condition, we check whether we know that i is non-negative so we can
learn something, namely that i >= len(n). Prove uses the poset to check whether
we know that i is non-negative. At this point the poset holds the following
relations as a directed graph:
-1 <= i <= 0
-1 < 0
In poset.OrderedOrEqual, we are testing for 0 <= i. In this case, condition (3)
above is true because there is a non-strict path from i -> 0, and that path
does NOT have any strict edges. Because this condition is true, the poset
reports to prove that i is known to be >= 0. Knowing, incorrectly, that i >= 0,
prove learns from the failed bounds check that i >= len(n) in the signed domain.
When the slice, n, was created, prove learned that len(n) == 1. Because i is
also the induction variable for the loop, upon entering the loop, prove previously
learned that i is in [-1,0]. So when prove attempts to learn from the failed
bounds check, it finds the new fact, i > len(n), unsatisfiable given that it
previously learned that i <= 0 and len(n) = 1.
Fixes#34802
Change-Id: I235f4224bef97700c3aa5c01edcc595eb9f13afc
Reviewed-on: https://go-review.googlesource.com/c/go/+/200759
Run-TryBot: Zach Jones <zachj1@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Keith Randall <khr@golang.org>
Increases the exec timeout from 5sec to 1min, but
also print out the error value on any test failure.
Fixes#34836
Change-Id: Ida2b8bd460243491ef0f90dfe0f978dfe02a0703
Reviewed-on: https://go-review.googlesource.com/c/go/+/200519
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Test case with code that caused a gccgo error while emitting export
data for an inlinable function.
Updates #34577.
Change-Id: I28b598c4c893c77f4a76bb4f2d27e5b42f702992
Reviewed-on: https://go-review.googlesource.com/c/go/+/198057
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently, escape analysis is able to record at most one dereference
when a parameter leaks to the heap; that is, at call sites, it can't
distinguish between any of these three functions:
func x1(p ****int) { sink = *p }
func x2(p ****int) { sink = **p }
func x3(p ****int) { sink = ***p }
Similarly, it's limited to recording parameter leaks to only the first
4 parameters, and only up to 6 dereferences.
All of these limitations are due to the awkward encoding scheme used
at the moment.
This CL replaces the encoding scheme with a simple [8]uint8 array,
which can handle up to the first 7 parameters, and up to 254
dereferences, which ought to be enough for anyone. And if not, it's
much more easily increased.
Shrinks export data size geometric mean for Kubernetes by 0.07%.
Fixes#33981.
Change-Id: I10a94b9accac9a0c91490e0d6d458316f5ca1e13
Reviewed-on: https://go-review.googlesource.com/c/go/+/197680
Reviewed-by: Cherry Zhang <cherryyz@google.com>
CL 170950 had a regression that makes the compiler produce
an invalid wasm binary if the data section is too large.
Loading such a binary gives the following error:
"LinkError: WebAssembly.instantiate(): data segment is out of bounds"
This change fixes the issue by ensuring that the minimum size of the
linear memory is larger than the end of the data section.
Fixes#34395.
Change-Id: I0c8629de7ffd0d85895ad31bf8c9d45fef197a57
Reviewed-on: https://go-review.googlesource.com/c/go/+/199358
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We're allowed to remove a write barrier when both the old
value in memory and the new value we're writing are not heap pointers.
Improve both those checks a little bit.
A pointer is known to not be a heap pointer if it is read from
read-only memory. This sometimes happens for loads of pointers
from string constants in read-only memory.
Do a better job of tracking which parts of memory are known to be
zero. Before we just kept track of a range of offsets in the most
recently allocated object. For code that initializes the new object's
fields in a nonstandard order, that tracking is imprecise. Instead,
keep a bit map of the first 64 words of that object, so we can track
precisely what we know to be zeroed.
The new scheme is only precise up to the first 512 bytes of the object.
After that, we'll use write barriers unnecessarily. Hopefully most
initializers of large objects will use typedmemmove, which does only one
write barrier check for the whole initialization.
Fixes#34723
Update #21561
Change-Id: Idf6e1b7d525042fb67961302d4fc6f941393cac8
Reviewed-on: https://go-review.googlesource.com/c/go/+/199558
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
For commuting ops, check whether the second argument is dead before
checking if the first argument is rematerializeable. Reusing the register
holding a dead value is always best.
Fixes#33580
Change-Id: I7372cfc03d514e6774d2d9cc727a3e6bf6ce2657
Reviewed-on: https://go-review.googlesource.com/c/go/+/199559
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
CL 188317 introduced a compiler crash during dwarf generation which
was reported as Issue #34520. After CL 188217, the issue appears to be
fixed. Add a testcase to avoid future regressions.
Fixes#34520
Change-Id: I73544a9e9baf8dbfb85c19eb6d202beea05affb6
Reviewed-on: https://go-review.googlesource.com/c/go/+/198546
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Nilcheck would move statements from NilCheck values to others that
turned out were already dead, which leads to lost statements. Better
to eliminate the dead code first.
One "error" is removed from test/prove.go because the code is
actually dead, and the additional deadcode pass removes it before
prove can run.
Change-Id: If75926ca1acbb59c7ab9c8ef14d60a02a0a94f8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/198479
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
During package initialization, the compiler tries to optimize:
var A = "foo"
var B = A
into
var A = "foo"
var B = "foo"
so that we can statically initialize both A and B and skip emitting
dynamic initialization code to assign "B = A".
However, this isn't safe in the presence of cmd/link's -X flag, which
might overwrite an initialized string-typed variable at link time. In
particular, if cmd/link changes A's static initialization, it won't
know it also needs to change B's static initialization.
To address this, this CL disables this optimization for string-typed
variables.
Fixes#34675.
Change-Id: I1c18f3b855f6d7114aeb39f96aaaf1b452b88236
Reviewed-on: https://go-review.googlesource.com/c/go/+/198657
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
By lazily starting the signal watch loop only on Notify,
we are able to have deadlock detection even when
"os/signal" is imported.
Thanks to Ian Lance Taylor for the solution and discussion.
With this change in, fix a runtime gorountine count test that
assumed that os/signal.init would unconditionally start the
signal watching goroutine, but alas no more.
Fixes#21576.
Change-Id: I6eecf82a887f59f2ec8897f1bcd67ca311ca42ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/101036
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
ORUNESTR represents the special case of integer->string conversion. If
the integer is a constant, then the string is a constant too, so
evconst needs to perform constant folding here.
Passes toolstash-check.
Fixes#34563.
Change-Id: Ieab3d76794d8ce570106b6b707a4bcd725d156e9
Reviewed-on: https://go-review.googlesource.com/c/go/+/197677
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
prove wasn't able to detect induction variables that was bound
by another inducation variable. This happened because an indvar
is a Phi, and thus in case of a dependency, the loop bounding
condition looked as Phi < Phi. This triggered an existing
codepath that checked whether the upper bound was a Phi to
detect loop conditions written in reversed order respect to the
idiomatic way (eg: for i:=0; len(n)>i; i++).
To fix this, we call the indvar pattern matching on both operands
of the loop condition, so that the first operand that matches
will be treated as the indvar.
Updates #24660 (removes a boundcheck from Fannkuch)
Change-Id: Iade83d8deb54f14277ed3f2e37b190e1ed173d11
Reviewed-on: https://go-review.googlesource.com/c/go/+/195220
Reviewed-by: David Chase <drchase@google.com>
Previously, we would recognize &(T{...}) expressions during type
checking, rewrite them into (*T){...}, and then do a lot of extra work
to make sure the user doesn't write (*T){...} themselves and
resynthesizing the OPTRLIT later on.
This CL simply handles &T{...} directly in the straight forward
manner, by changing OADDR directly to OPTRLIT when appropriate.
While here, match go/types's invalid composite literal type error
message.
Passes toolstash-check.
Change-Id: I902b14c7e2cd9fa93e6915dd58272d2352ba38f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/197120
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Now that OpSliceMake is called by runtime.makeslice callers,
prove can see and record the actual length and cap of each
slice being constructed.
This small patch is enough to remove 260 additional bound checks
from cmd+std.
Thanks to Martin Möhrmann for pointing me to CL141822 that
I had missed.
Updates #24660
Change-Id: I14556850f285392051f3f07d13b456b608b64eb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/196784
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Add a bunch of extra tests and benchmarks for defer, in preparation for new
low-cost (open-coded) implementation of defers (see #34481),
- New file defer_test.go that tests a bunch more unusual defer scenarios,
including things that might have problems for open-coded defers.
- Additions to callers_test.go actually verifying what the stack trace looks like
for various panic or panic-recover scenarios.
- Additions to crash_test.go testing several more crash scenarios involving
recursive panics.
- New benchmark in runtime_test.go measuring speed of panic-recover
- New CGo benchmark in cgo_test.go calling from Go to C back to Go that
shows defer overhead
Updates #34481
Change-Id: I423523f3e05fc0229d4277dd00073289a5526188
Reviewed-on: https://go-review.googlesource.com/c/go/+/197017
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Currently, when we create an OPTRLIT node, it defaults to the
OCOMPLIT's final element's position. But it improves error messages to
use the OCOMPLIT's own position instead.
Change-Id: Ibb031f543c7248d88d99fd0737685e01d86e2500
Reviewed-on: https://go-review.googlesource.com/c/go/+/197119
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This commit just adds a regress test for a few of the important corner
cases that I identified in #27557, which turn out to not be tested
anywhere.
While here, annotate a few of the existing test cases where we could
improve escape analysis.
Updates #27557.
Change-Id: Ie57792a538f7899bb17915485fabc86100f469a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/197137
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
On modern 64bit CPUs a SHR, SHL or AND instruction take 1 cycle to execute.
A pair of shifts that operate on the same register will take 2 cycles
and needs to wait for the input register value to be available.
Large constants used to mask the high bits of a register with an AND
instruction can not be encoded as an immediate in the AND instruction
on amd64 and therefore need to be loaded into a register with a MOV
instruction.
However that MOV instruction is not dependent on the output register and
on many CPUs does not compete with the AND or shift instructions for
execution ports.
Using a pair of shifts to mask high bits instead of an AND to mask high
bits of a register has a shorter encoding and uses one less general
purpose register but is slower due to taking one clock cycle longer
if there is no register pressure that would make the AND variant need to
generate a spill.
For example the instructions emitted for (x & 1 << 63) before this CL are:
48c1ea3f SHRQ $0x3f, DX
48c1e23f SHLQ $0x3f, DX
after this CL the instructions are the same as GCC and LLVM use:
48b80000000000000080 MOVQ $0x8000000000000000, AX
4821d0 ANDQ DX, AX
Some platforms such as arm64 already have SSA optimization rules to fuse
two shift instructions back into an AND.
Removing the general rule to rewrite AND to SHR+SHL speeds up this benchmark:
var GlobalU uint
func BenchmarkAndHighBits(b *testing.B) {
x := uint(0)
for i := 0; i < b.N; i++ {
x &= 1 << 63
}
GlobalU = x
}
amd64/darwin on Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz:
name old time/op new time/op delta
AndHighBits-4 0.61ns ± 6% 0.42ns ± 6% -31.42% (p=0.000 n=25+25):
'go run run.go -all_codegen -v codegen' passes with following adjustments:
ARM64: The BFXIL pattern ((x << lc) >> rc | y & ac) needed adjustment
since ORshiftRL generation fusing '>> rc' and '|' interferes
with matching ((x << lc) >> rc) to generate UBFX. Previously
ORshiftLL was created first using the shifts generated for (y & ac).
S390X: Add rules for abs and copysign to match use of AND instead of SHIFTs.
Updates #33826
Updates #32781
Change-Id: I5a59f6239660d53c029cd22dfb44ddf39f93a56c
Reviewed-on: https://go-review.googlesource.com/c/go/+/196810
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
On modern 64bit CPUs a SHR, SHL or AND instruction take 1 cycle to execute.
A pair of shifts that operate on the same register will take 2 cycles
and needs to wait for the input register value to be available.
Large constants used to mask the high bits of a register with an AND
instruction can not be encoded as an immediate in the AND instruction
on amd64 and therefore need to be loaded into a register with a MOV
instruction.
However that MOV instruction is not dependent on the output register and
on many CPUs does not compete with the AND or shift instructions for
execution ports.
Using a pair of shifts to mask high bits instead of an AND to mask high
bits of a register has a shorter encoding and uses one less general
purpose register but is slower due to taking one clock cycle longer
if there is no register pressure that would make the AND variant need to
generate a spill.
For example the instructions emitted for (x & 1 << 63) before this CL are:
48c1ea3f SHRQ $0x3f, DX
48c1e23f SHLQ $0x3f, DX
after this CL the instructions are the same as GCC and LLVM use:
48b80000000000000080 MOVQ $0x8000000000000000, AX
4821d0 ANDQ DX, AX
Some platforms such as arm64 already have SSA optimization rules to fuse
two shift instructions back into an AND.
Removing the general rule to rewrite AND to SHR+SHL speeds up this benchmark:
var GlobalU uint
func BenchmarkAndHighBits(b *testing.B) {
x := uint(0)
for i := 0; i < b.N; i++ {
x &= 1 << 63
}
GlobalU = x
}
amd64/darwin on Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz:
name old time/op new time/op delta
AndHighBits-4 0.61ns ± 6% 0.42ns ± 6% -31.42% (p=0.000 n=25+25):
'go run run.go -all_codegen -v codegen' passes with following adjustments:
ARM64: The BFXIL pattern ((x << lc) >> rc | y & ac) needed adjustment
since ORshiftRL generation fusing '>> rc' and '|' interferes
with matching ((x << lc) >> rc) to generate UBFX. Previously
ORshiftLL was created first using the shifts generated for (y & ac).
S390X: Add rules for abs and copysign to match use of AND instead of SHIFTs.
Updates #33826
Updates #32781
Change-Id: I43227da76b625de03fbc51117162b23b9c678cdb
Reviewed-on: https://go-review.googlesource.com/c/go/+/194297
Run-TryBot: Martin Möhrmann <martisch@uos.de>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
i32.eqz instructions don't appear unless needed in if conditions anymore
after CL 195204. I forgot to run the codegen tests while submitting the CL.
Thanks to @martisch for catching it.
Fixes#34442
Change-Id: I177b064b389be48e39d564849714d7a8839be13e
Reviewed-on: https://go-review.googlesource.com/c/go/+/196580
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
When compiling expression switches, we try to optimize runs of
constants into binary searches. The ordering used isn't visible to the
application, so it's unimportant as long as we're consistent between
sorting and searching.
For strings, it's much cheaper to compare string lengths than strings
themselves, so instead of ordering strings by "si <= sj", we currently
order them by "len(si) < len(sj) || len(si) == len(sj) && si <= sj"
(i.e., the lexicographical ordering on the 2-tuple (len(s), s)).
However, it's also somewhat cheaper to compare strings for equality
(i.e., ==) than for ordering (i.e., <=). And if there were two or
three string constants of the same length in a switch statement, we
might unnecessarily emit ordering comparisons.
For example, given:
switch s {
case "", "1", "2", "3": // ordered by length then content
goto L
}
we currently compile this as:
if len(s) < 1 || len(s) == 1 && s <= "1" {
if s == "" { goto L }
else if s == "1" { goto L }
} else {
if s == "2" { goto L }
else if s == "3" { goto L }
}
This CL switches to using a 2-level binary search---first on len(s),
then on s itself---so that string ordering comparisons are only needed
when there are 4 or more strings of the same length. (4 being the
cut-off for when using binary search is actually worthwhile.)
So the above switch instead now compiles to:
if len(s) == 0 {
if s == "" { goto L }
} else if len(s) == 1 {
if s == "1" { goto L }
else if s == "2" { goto L }
else if s == "3" { goto L }
}
which is better optimized by walk and SSA. (Notably, because there are
only two distinct lengths and no more than three strings of any
particular length, this example ends up falling back to simply using
linear search.)
Test case by khr@ from CL 195138.
Fixes#33934.
Change-Id: I8eeebcaf7e26343223be5f443d6a97a0daf84f07
Reviewed-on: https://go-review.googlesource.com/c/go/+/195340
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
golang.org/cl/109517 optimized the compiler to avoid the allocation for make in
append(x, make([]T, y)...). This was only implemented for the case that y has type int.
This change extends the optimization to trigger for all integer types where the value
is known at compile time to fit into an int.
name old time/op new time/op delta
ExtendInt-12 106ns ± 4% 106ns ± 0% ~ (p=0.351 n=10+6)
ExtendUint64-12 1.03µs ± 5% 0.10µs ± 4% -90.01% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
ExtendInt-12 0.00B 0.00B ~ (all equal)
ExtendUint64-12 13.6kB ± 0% 0.0kB -100.00% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
ExtendInt-12 0.00 0.00 ~ (all equal)
ExtendUint64-12 1.00 ± 0% 0.00 -100.00% (p=0.000 n=10+10)
Updates #29785
Change-Id: Ief7760097c285abd591712da98c5b02bc3961fcd
Reviewed-on: https://go-review.googlesource.com/c/go/+/182559
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This CL expands the test for #29612 to check that type switches also
work correctly when type hashes collide.
Change-Id: Ia153743e6ea0736c1a33191acfe4d8ba890be527
Reviewed-on: https://go-review.googlesource.com/c/go/+/195782
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Support for overlapping interfaces is a new (proposed) Go language
feature to be supported in Go 1.14, so it shouldn't be supported under
-lang=go1.13 or earlier.
Fixes#34329.
Change-Id: I5fea5716b7d135476980bc40b4f6e8c611b67735
Reviewed-on: https://go-review.googlesource.com/c/go/+/195678
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
This reverts CL 192101.
Reason for revert: The same paragraph was added 2 weeks ago
(look a few lines above)
Change-Id: I05efb2631d7b4966f66493f178f2a649c715a3cc
Reviewed-on: https://go-review.googlesource.com/c/go/+/195637
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This information is redundant with the position information already
provided. Also, no other -m diagnostics print out function name.
While here, report parameter leak diagnostics against the parameter
declaration position rather than the function, and use Warnl for
"moved to heap" messages.
Test cases updated programmatically by removing the first word from
every "no match for" error emitted by run.go:
go run run.go |& \
sed -E -n 's/^(.*):(.*): no match for `([^ ]* (.*))` in:$/\1!\2!\3!\4/p' | \
while IFS='!' read -r fn line before after; do
before=$(echo "$before" | sed 's/[.[\*^$()+?{|]/\\&/g')
after=$(echo "$after" | sed -E 's/(\&|\\)/\\&/g')
fn=$(find . -name "${fn}" | head -1)
sed -i -E -e "${line}s/\"${before}\"/\"${after}\"/" "${fn}"
done
Passes toolstash-check.
Change-Id: I6e02486b1409e4a8dbb2b9b816d22095835426b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/195040
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
It is useful to know about the -all_codegen option for running
codegen tests for all platforms. I was puzzling that some codegen
test was not failing on my local machine or on trybot, until I
found this option.
Change-Id: I062cf4d73f6a6c9ebc2258195779d2dab21bc36d
Reviewed-on: https://go-review.googlesource.com/c/go/+/192101
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change adds an intrinsic for Mul64 on s390x. To achieve that,
a new assembly instruction, MLGR, is introduced in s390x/asmz.go. This assembly
instruction directly uses an existing instruction on Z and supports multiplication
of two 64 bit unsigned integer and stores the result in two separate registers.
In this case, we require the multiplcand to be stored in register R3 and
the output result (the high and low 64 bit of the product) to be stored in
R2 and R3 respectively.
A test case is also added.
Benchmark:
name old time/op new time/op delta
Mul-18 11.1ns ± 0% 1.4ns ± 0% -87.39% (p=0.002 n=8+10)
Mul32-18 2.07ns ± 0% 2.07ns ± 0% ~ (all equal)
Mul64-18 11.1ns ± 1% 1.4ns ± 0% -87.42% (p=0.000 n=10+10)
Change-Id: Ieca6ad1f61fff9a48a31d50bbd3f3c6d9e6675c1
Reviewed-on: https://go-review.googlesource.com/c/go/+/194572
Reviewed-by: Michael Munday <mike.munday@ibm.com>
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Part of the general trend of moving yyerror calls out of walk and into
typecheck.
Notably, this requires splitting test/typeswitch2.go into two files,
because now some of the errors are reported during typecheck and
others are still reported during walk; and if there were any errors
during typecheck, then cmd/compile exits without invoking walk.
Passes toolstash-check.
Change-Id: I05ee0c00b99af659ee1eef098d342d0d736cf31e
Reviewed-on: https://go-review.googlesource.com/c/go/+/194659
Reviewed-by: Robert Griesemer <gri@golang.org>
This CL moves parameter tagging to before escape analysis is complete,
so we still have access to EscLocation. This will be useful once
EscLocation starts tracking higher-fidelity escape details.
Notably, this CL stops using n.Esc to record parameter escape analysis
details. Now escape analysis only ever sets n.Esc to EscNone or
EscHeap. (It still defaults to EscUnknown, and is set to EscNever in
some places though.)
Passes toolstash-check.
Updates #33981.
Change-Id: I50a91ea1e38c442092de6cd14e20b211f8f818c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/193178
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This CL gets rid of the MOVDreg and MOVDnop SSA operations on
s390x. They were originally inserted to help avoid situations
where a sign/zero extension was elided but a spill invalidated
the optimization. It's not really clear we need to do this though
(amd64 doesn't have these ops for example) so long as we are
careful when removing sign/zero extensions. Also, the MOVDreg
technique doesn't work if the register is spilled before the
MOVDreg op (I haven't seen that in practice).
Removing these ops reduces the complexity of the rules and also
allows us to unblock optimizations. For example, the compiler can
now merge the loads in binary.{Big,Little}Endian.PutUint16 which
it wasn't able to do before. This CL reduces the size of the .text
section in the go tool by about 4.7KB (0.09%).
Change-Id: Icaddae7f2e4f9b2debb6fabae845adb3f73b41db
Reviewed-on: https://go-review.googlesource.com/c/go/+/173897
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Previously, we used a single "untyped number" type for all untyped
numeric constants. This led to vague error messages like "string(1.0)"
reporting that "1 (type untyped number)" can't be converted to string,
even though "string(1)" is valid.
This CL makes cmd/compile more like go/types by utilizing
types.Ideal{int,rune,float,complex} instead of types.Types[TIDEAL],
and keeping n.Type in sync with n.Val().Ctype() during constant
folding.
Thanks to K Heller for looking into this issue, and for the included
test case.
Fixes#21979.
Change-Id: Ibfea88c05704bc3c0a502a455d018a375589754d
Reviewed-on: https://go-review.googlesource.com/c/go/+/194019
Reviewed-by: Robert Griesemer <gri@golang.org>
On modern 64bit CPUs a SHR, SHL or AND instruction take 1 cycle to execute.
A pair of shifts that operate on the same register will take 2 cycles
and needs to wait for the input register value to be available.
Large constants used to mask the high bits of a register with an AND
instruction can not be encoded as an immediate in the AND instruction
on amd64 and therefore need to be loaded into a register with a MOV
instruction.
However that MOV instruction is not dependent on the output register and
on many CPUs does not compete with the AND or shift instructions for
execution ports.
Using a pair of shifts to mask high bits instead of an AND to mask high
bits of a register has a shorter encoding and uses one less general
purpose register but is slower due to taking one clock cycle longer
if there is no register pressure that would make the AND variant need to
generate a spill.
For example the instructions emitted for (x & 1 << 63) before this CL are:
48c1ea3f SHRQ $0x3f, DX
48c1e23f SHLQ $0x3f, DX
after this CL the instructions are the same as GCC and LLVM use:
48b80000000000000080 MOVQ $0x8000000000000000, AX
4821d0 ANDQ DX, AX
Some platforms such as arm64 already have SSA optimization rules to fuse
two shift instructions back into an AND.
Removing the general rule to rewrite AND to SHR+SHL speeds up this benchmark:
var GlobalU uint
func BenchmarkAndHighBits(b *testing.B) {
x := uint(0)
for i := 0; i < b.N; i++ {
x &= 1 << 63
}
GlobalU = x
}
amd64/darwin on Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz:
name old time/op new time/op delta
AndHighBits-4 0.61ns ± 6% 0.42ns ± 6% -31.42% (p=0.000 n=25+25):
Updates #33826
Updates #32781
Change-Id: I862d3587446410c447b9a7265196b57f85358633
Reviewed-on: https://go-review.googlesource.com/c/go/+/191780
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Use the following (suboptimal) script to obtain a list of possible
typos:
#!/usr/bin/env sh
set -x
git ls-files |\
grep -e '\.\(c\|cc\|go\)$' |\
xargs -n 1\
awk\
'/\/\// { gsub(/.*\/\//, ""); print; } /\/\*/, /\*\// { gsub(/.*\/\*/, ""); gsub(/\*\/.*/, ""); }' |\
hunspell -d en_US -l |\
grep '^[[:upper:]]\{0,1\}[[:lower:]]\{1,\}$' |\
grep -v -e '^.\{1,4\}$' -e '^.\{16,\}$' |\
sort -f |\
uniq -c |\
awk '$1 == 1 { print $2; }'
Then, go through the results manually and fix the most obvious typos in
the non-vendored code.
Change-Id: I3cb5830a176850e1a0584b8a40b47bde7b260eae
Reviewed-on: https://go-review.googlesource.com/c/go/+/193848
Reviewed-by: Robert Griesemer <gri@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>
This reverts commit 2da9c3e0f9.
Reason for revert: while the new error messages are more informative,
they're not strictly correct. This CL also conflicts with CL 187657.
Change-Id: I1c36cf7e86c2f35ee83a4f98918ee38aa1f59965
Reviewed-on: https://go-review.googlesource.com/c/go/+/193977
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Follow-up to Change-Id: If6e52c59eab438599d641ecf6f110ebafca740a9
This addresses the remaining tech debt on issue 21979.
The aforementioned previous CL silenced one of two mostly redundant
compiler errors. However, the silenced error was the more expressive
error. This CL now imbues the surviving error with the same level
of expressiveness as the old semi-redundant error.
Fixes#21979
Change-Id: I3273d48c88bbab073fabe53421d801df621ce321
Reviewed-on: https://go-review.googlesource.com/c/go/+/191079
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Test with some code that triggered a compilation error bug in gccgo.
Updates #33866.
Change-Id: Ib2f226bbbebbfae33b41037438fe34dc5f2ad034
Reviewed-on: https://go-review.googlesource.com/c/go/+/193261
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Add block method to preserve loop depth when evaluating statements in a
block, so escape analysis can handle looping label more precisely.
Updates #22438
Change-Id: I39b306544a6c0ee3fcbebbe0d0ee735cb71773e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/193517
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Right now we generate hash functions for all types, just in case they
are used as map keys. That's a lot of wasted effort and binary size
for types which will never be used as a map key. Instead, generate
hash functions only for types that we know are map keys.
Just doing that is a bit too simple, since maps with an interface type
as a key might have to hash any concrete key type that implements that
interface. So for that case, implement hashing of such types at
runtime (instead of with generated code). It will be slower, but only
for maps with interface types as keys, and maybe only a bit slower as
the aeshash time probably dominates the dispatch time.
Reorg where we keep the equals and hash functions. Move the hash function
from the key type to the map type, saving a field in every non-map type.
That leaves only one function in the alg structure, so get rid of that and
just keep the equal function in the type descriptor itself.
cmd/go now has 10 generated hash functions, instead of 504. Makes
cmd/go 1.0% smaller. Update #6853.
Speed on non-interface keys is unchanged. Speed on interface keys
is ~20% slower:
name old time/op new time/op delta
MapInterfaceString-8 23.0ns ±21% 27.6ns ±14% +20.01% (p=0.002 n=10+10)
MapInterfacePtr-8 19.4ns ±16% 23.7ns ± 7% +22.48% (p=0.000 n=10+8)
Change-Id: I7c2e42292a46b5d4e288aaec4029bdbb01089263
Reviewed-on: https://go-review.googlesource.com/c/go/+/191198
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
Assinging to 1-element array/1-field struct variable is considered clobbering
the whole variable. By emitting OpVarDef in this case, liveness analysis
can now know the variable is redefined.
Also, the isfat is not necessary anymore, and will be removed in follow up CL.
Fixes#33916
Change-Id: Iece0d90b05273f333d59d6ee5b12ee7dc71908c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/192979
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
In general, a conversion to interface type may require values to be
boxed, which in turn necessitates escape analysis to determine whether
the boxed representation can be stack allocated.
However, esc.go used to unconditionally print escape analysis
decisions about OCONVIFACE, even for conversions that don't require
boxing (e.g., pointers, channels, maps, functions).
For test compatibility with esc.go, escape.go similarly printed these
useless diagnostics. This CL removes the diagnostics, and updates test
expectations accordingly.
Change-Id: I97c57a4a08e44d265bba516c78426ff4f2bf1e12
Reviewed-on: https://go-review.googlesource.com/c/go/+/192697
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This CL extends {defer,resume}checkwidth to support nesting, which
simplifies usage.
Updates #33658.
Change-Id: Ib3ffb8a7cabfae2cbeba74e21748c228436f4726
Reviewed-on: https://go-review.googlesource.com/c/go/+/192721
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
For performance reasons (avoiding costly cross-compilations) CL 177577
changed the codegen test harness to only run the tests for the
machine's GOARCH by default.
This change updates the codegen README accordingly, explaining what
all.bash does run by default and how to perform the tests for all
architectures.
Fixes#33924
Change-Id: I43328d878c3e449ebfda46f7e69963a44a511d40
Reviewed-on: https://go-review.googlesource.com/c/go/+/192619
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
This CL eliminates unnecessary pairs of I32WrapI64 and
I64ExtendI32U generated by the WASM backend for IF
statements. And it makes the total size of pkg/js_wasm/
decreases about 490KB.
Change-Id: I16b0abb686c4e30d5624323166ec2d0ec57dbe2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/191758
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Richard Musiol <neelance@gmail.com>
This CL reverts CL 192097 and fixes the issue in CL 189277.
Change-Id: Icd271262e1f5019a8e01c91f91c12c1261eeb02b
Reviewed-on: https://go-review.googlesource.com/c/go/+/192519
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Raising an out-of-bounds panic is confusing. There's no indication
that the underlying problem is a race.
The runtime already does a pretty good job of detecting this kind of
race (modification while iterating). We might as well just reorganize
a bit to avoid the out-of-bounds panic.
Fixes#33275
Change-Id: Icdd337ad2eb3c84f999db0850ec1d2ff2c146b6e
Reviewed-on: https://go-review.googlesource.com/c/go/+/191197
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
- only convert literal strings if there were no syntax errors
(some of the conversion routines exit if there is an error)
- mark nodes for literals with syntax errors to avoid follow-on
errors
- don't attempt to import packages whose path had syntax errors
Fixes#32133.
Change-Id: I1803ad48c65abfecf6f48ddff1e27eded5e282c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/192437
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The existing pointer comparison optimizations
don't include pointer arithmetic. Add them.
These rules trigger a few times in std cmd, while compiling:
time.Duration.String
cmd/go/internal/tlog.NodeHash
crypto/tls.ticketKeyFromBytes (3 times)
crypto/elliptic.(*p256Point).p256ScalarMult (15 times!)
crypto/elliptic.initTable
These weird comparisons occur when using the copy builtin,
which does a pointer comparison between src and dst.
This also happens to fix#32454, by optimizing enough
early on that all values can be eliminated.
Fixes#32454
Change-Id: I799d45743350bddd15a295dc1e12f8d03c11d1c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/180940
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
The original report in #5172 was that cmd/compile was generating bogus
follow-on error messages when typechecking a struct failed. Instead of
fixing those follow-on error messages, golang.org/cl/9614044 suppress all
follow-on error messages after struct typecheck fails. We should
continue emitting error messages instead.
While at it, also add the test case for original report.
Fixes#33947
Change-Id: I4a5c6878977128abccd704350a12df743631c7bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/191944
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Complex type is the only TIDEAL that lack of support for all comparison
operators. When rewriting constant comparison into literal node, that
missing cause compiler raise an internal error.
Checking the operator is available for complex type before that fix the
problem.
We can make this check works more generally if there's more type lack of
supporting all comparison operators added, but it does not seem to be
happened, so just check explicitly for complex only.
Fixes#32723
Change-Id: I4938b1bdcbcdae9a9d87436024984bd2ab12995e
Reviewed-on: https://go-review.googlesource.com/c/go/+/183459
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
typecheck only set n.Type.Nod for declared type, and leave it nil for
anonymous types, type alias. It leads to compiler crashes, because
n.Type.Nod is nil at the time dowidth was called.
Fixing it by set n.Type.Nod right after n.Type initialization if n.Op is
OTYPE.
When embedding interface cycles involve in type alias, it also helps
pointing the error message to the position of the type alias
declaration, instead of position of embedding interface.
Fixes#31872
Change-Id: Ia18391e987036a91f42ba0c08b5506f52d07f683
Reviewed-on: https://go-review.googlesource.com/c/go/+/191540
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Testcase for a gollvm bug (assert in Llvm_backend::materializeComposite).
Updates golang/go#33020.
Change-Id: Icdf5b4b2b6eb55a5b48a31a61c41215b1ae4cf01
Reviewed-on: https://go-review.googlesource.com/c/go/+/191743
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The syntax of a shifted operation does not have a "$" sign for
the shift amount. Remove it.
Change-Id: I50782fe942b640076f48c2fafea4d3175be8ff99
Reviewed-on: https://go-review.googlesource.com/c/go/+/192100
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This will improve liveness analysis slightly, the same logic as
isdirectiface curently does. In:
type T struct {
m map[int]int
}
v := T{}
v.m = make(map[int]int)
T is considered "fat", now it is not. So assigning to v.m is considered
to clobber the entire v.
This is follow up of CL 179057.
Change-Id: Id6b4807b8e8521ef5d8bcb14fedb6dceb9dbf18c
Reviewed-on: https://go-review.googlesource.com/c/go/+/179578
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
In interface-to-concrete comparisons, we are short circuiting on the interface
value's dynamic type before evaluating the concrete expression for side effects,
causing concrete expression won't panic at runtime, while it should.
To fix it, evaluating the RHS of comparison before we do the short-circuit.
We also want to prioritize panics in the LHS over the RHS, so evaluating
the LHS too.
Fixes#32187
Change-Id: I15b58a523491b7fd1856b8fdb9ba0cba5d11ebb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/178817
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Prep for subsequent CLs to remove old escape analysis pass.
This CL removes -newescape=true from tests that use it, and deletes
tests that use -newescape=false. (For history, see CL 170447.)
Notably, this removes escape_because.go without any replacement, but
this is being tracked by #31489.
Change-Id: I6f6058d58fff2c5d210cb1d2713200cc9f501ca7
Reviewed-on: https://go-review.googlesource.com/c/go/+/187617
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Extend the optimization introduced in CL 141118 to the wasm architecture.
And for reference, the rules trigger 212 times while building std and cmd
$GOOS=js GOARCH=wasm gotip build std cmd
$grep -E "Wasm.rules:44(1|2|3|4)" rulelog | wc -l
212
Updates #26498
Change-Id: I153684a2b98589ae812b42268da08b65679e09d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/185477
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Richard Musiol <neelance@gmail.com>
Use the shiftIsBounded function to generate more efficient
Shift instructions.
Updates #25167
Change-Id: Id350f8462dc3a7ed3bfed0bcbea2860b8f40048a
Reviewed-on: https://go-review.googlesource.com/c/go/+/182558
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Richard Musiol <neelance@gmail.com>
This CL optimizes math.bits.TrailingZeros16 on 386 with
a pair of BSFL and ORL instrcutions.
The case TrailingZeros16-4 of the benchmark test in
math/bits shows big improvement.
name old time/op new time/op delta
TrailingZeros16-4 1.55ns ± 1% 0.87ns ± 1% -43.87% (p=0.000 n=50+49)
Change-Id: Ia899975b0e46f45dcd20223b713ed632bc32740b
Reviewed-on: https://go-review.googlesource.com/c/go/+/189277
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This CL add test cases for the unary FP negative
operation.
Change-Id: I54e7292ca9df05da0c2b113adefc97ee1e94c6e2
Reviewed-on: https://go-review.googlesource.com/c/go/+/190937
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This CL performs the branchelim optimization on WASM with its
select instruction. And the total size of pkg/js_wasm decreased
about 80KB by this optimization.
Change-Id: I868eb146120a1cac5c4609c8e9ddb07e4da8a1d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/190957
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Richard Musiol <neelance@gmail.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
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>
The assembly output for x & c == c, where c is power of 2:
MOVQ "".set+8(SP), AX
ANDQ $8, AX
CMPQ AX, $8
SETEQ "".~r2+24(SP)
With optimization using bitset:
MOVQ "".set+8(SP), AX
BTL $3, AX
SETCS "".~r2+24(SP)
output less than 1 instruction.
However, there is no speed improvement:
name old time/op new time/op delta
AllBitSet-8 0.35ns ± 0% 0.35ns ± 0% ~ (all equal)
Fixes#31904
Change-Id: I5dca4e410bf45716ed2145e3473979ec997e35d4
Reviewed-on: https://go-review.googlesource.com/c/go/+/175957
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Array accesses with index types smaller than the machine word size may
involve a sign or zero extension of the index value before bounds
checking. Currently, this defeats prove because the facts about the
original index value don't flow through the sign/zero extension.
This CL fixes this by looking back through value-preserving sign/zero
extensions when adding facts via Update and, where appropriate, applying
the same facts using the pre-extension value. This fix is enhanced by
also looking back through value-preserving extensions within
ft.isNonNegative to infer whether the extended value is known to be
non-negative. Without this additional isNonNegative enhancement, this
logic is rendered significantly less effective by the limitation
discussed in the next paragraph.
In Update, the application of facts to pre-extension values is limited
to cases where the domain of the new fact is consistent with the type of
the pre-extension value. There may be cases where this cross-domain
passing of facts is valid, but distinguishing them from the invalid
cases is difficult for me to reason about and to implement.
Assessing which cases to allow requires details about the context and
inferences behind the fact being applied which are not available
within Update. Additional difficulty arises from the fact that the SSA
does not curently differentiate extensions added by the compiler for
indexing operations, extensions added by the compiler for implicit
conversions, or explicit extensions from the source.
Examples of some cases that would need to be filtered correctly for
cross-domain facts:
(1) A uint8 is zero-extended to int for indexing (a value-preserving
zeroExt). When, if ever, can signed domain facts learned about the int be
applied to the uint8?
(2) An int8 is sign-extended to int16 (value-preserving) for an equality
comparison. Equality comparison facts are currently always learned in both
the signed and unsigned domains. When, if ever, can the unsigned facts
learned about the int16, from the int16 != int16 comparison, be applied
to the original int8?
This is an alternative to CL 122695 and CL 174309. Compared to CL 122695,
this CL differs in that the facts added about the pre-extension value will
pass through the Update method, where additional inferences are processed
(e.g. fence-post implications, see #29964). CL 174309 is limited to bounds
checks, so is narrower in application, and makes the code harder to read.
Fixes#26292.
Fixes#29964.
Fixes#15074
Removes 238 bounds checks from std/cmd.
Change-Id: I1f87c32ee672bfb8be397b27eab7a4c2f304893f
Reviewed-on: https://go-review.googlesource.com/c/go/+/174704
Run-TryBot: Zach Jones <zachj1@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Don't skip closing parentheses of any kind after a missing
expression. They are likely part of the lexical construct
enclosing the expression.
Fixes#33386.
Change-Id: Ic0abc2037ec339a345ec357ccc724b7ad2a64c00
Reviewed-on: https://go-review.googlesource.com/c/go/+/188502
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Quietly drop duplicate methods inherited from embedded interfaces if
they have an identical signature to existing methods.
Updates #6977.
Change-Id: I144151cb7d99695f12b555c0db56207993c56284
Reviewed-on: https://go-review.googlesource.com/c/go/+/187519
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Quietly drop duplicate methods from embedded interfaces
if they have an identical signature to existing methods.
Instead of adjusting the prior syntax-based only method set
computation where methods don't have signature information
(and thus where de-duplication according to the new rules
would have been somewhat tricky to get right), this change
completely rewrites interface method set computation, taking
a page from the cmd/compiler's implementation. In a first
pass, when type-checking interfaces, explicit methods and
embedded interfaces are collected, but the interfaces are
not "expanded", that is the final method set computation
is done lazily, either when needed for method lookup, or
at the end of type-checking.
While this is a substantial rewrite, it allows us to get
rid of the separate (duplicate and delicate) syntactical
method set computation and generally simplifies checking
of interface types significantly. A few (esoteric) test
cases now have slightly different error messages but all
tests that are accepted by cmd/compile are also accepted
by go/types.
(This is a replacement for golang.org/cl/190258.)
Updates #6977.
Change-Id: Ic8b9321374ab4f617498d97c12871b69d1119735
Reviewed-on: https://go-review.googlesource.com/c/go/+/191257
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
There is real (albeit generated) code that exceeds the limit.
Fixes#33555
Change-Id: I668e85825d3d2a471970e869abe63f3492213cc1
Reviewed-on: https://go-review.googlesource.com/c/go/+/189697
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
The compiler can crash if the compiled code tries to
unconditionally read from a nil pointer. This should cause
the generated binary to panic, not the compiler.
Fixes#33438
Change-Id: Ic8fa89646d6968e2cc4e27da0ad9286662f8bc49
Reviewed-on: https://go-review.googlesource.com/c/go/+/188760
Reviewed-by: Austin Clements <austin@google.com>
We shouldn't mask to desired registers if we haven't masked out all the
forbidden registers yet. In this path we haven't masked out the nospill
registers yet. If the resulting mask contains only nospill registers, then
allocReg fails.
This can only happen on resultNotInArgs-marked instructions, which exist
only on the ARM64, MIPS, MIPS64, and PPC64 ports.
Maybe there's a better way to handle resultNotInArgs instructions.
But for 1.13, this is a low-risk fix.
Fixes#33355
Change-Id: I1082f78f798d1371bde65c58cc265540480e4fa4
Reviewed-on: https://go-review.googlesource.com/c/go/+/188178
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Gccgo generates incorrect type equality functions for some types.
CL 185817 fixes it. This CL adds a test.
Updates #33062.
Change-Id: Id445c5d44a437512c65c46a029e49b7fc32e4d89
Reviewed-on: https://go-review.googlesource.com/c/go/+/185818
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Updates #33013
Change-Id: I3db062b37860bb0c6c99a553408b47cf0313531e
Reviewed-on: https://go-review.googlesource.com/c/go/+/185517
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Gccgo CL 184998 added optimizations for one- and two-case select
statements. But it didn't handle break statement in the select
case correctly. The fix is CL 185519. This CL adds a test.
Change-Id: Ide1b199f106172b41dd77c1f6e0d662fccdd8cc5
Reviewed-on: https://go-review.googlesource.com/c/go/+/185520
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
For OLSH/ORSH, the right node is not a uintptr-typed. However,
unsafeValue still be called recursively for it, causing the
compiler crashes.
To fixing, the right node only needs to be evaluated
for side-effects, so just discard its value.
Fixes#32959
Change-Id: I34d5aa0823a0545f6dad1ec34774235ecf11addc
Reviewed-on: https://go-review.googlesource.com/c/go/+/185039
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Test case that causes incorrect compiler error from gccgo.
Updates #32922
Change-Id: I59432a8e8770cf03eda293f6d110c081c18fa88b
Reviewed-on: https://go-review.googlesource.com/c/go/+/184918
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This CL adds a test for gccgo bug #32901: not all the type
descriptors are registered and thus deduplicated with types
created by reflection. It needs a few levels of indirect imports
to trigger this bug.
Updates #32901.
Change-Id: Idbd89bedd63fea746769f2687f3f31c9767e5ec0
Reviewed-on: https://go-review.googlesource.com/c/go/+/184718
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Test case that caused a compiler crash in gofrontend, related to
exporting inlinable function bodies.
Updates #32778
Change-Id: Iacf1753825d5359da43e5e281189876d4c3dd3c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/183851
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
It ends up making two similar types, [N]uint8 of both
alg and noalg varieties. Comparsions between the two then
don't come out equal when they should.
In particular, the type *[N]uint8 has an Elem pointer which
must point to one of the above two types; it can't point to both.
Thus allocating a *[N]uint8 and dereferencing it might be a
different type than a [N]uint8.
The fix is easy. Making a small test for this is really hard. It
requires that both a argless defer and the test be imported by a
common parent package. This is why a main binary doesn't see this
issue, but a test does (as Agniva noticed), because there's a wrapper
package that imports both the test and the defer.
Types like [N]uint8 don't really need to be marked noalg anyway,
as the generated code (if any) will be shared among all
vanilla memory types of the same size.
Fixes#32595
Change-Id: If7b77fa6ed56cd4495601c3f90170682d853b82f
Reviewed-on: https://go-review.googlesource.com/c/go/+/182357
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Adjusting gofrontend error messages for GCC standards causes the
messages expected by this test to be adjusted slightly: the gofrontend
code now quotes the _ identifier.
Change-Id: I55ee2ae70b4da3bf7a421ceea80b254dd17601a9
Reviewed-on: https://go-review.googlesource.com/c/go/+/183477
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
A missing operand to mergePoint caused lower to place values
in the wrong blocks.
Includes test, belt+suspenders to do both ssa check and verify
the output (was is how the bug was originally observed).
The fixed bug here is very likely present in Go versions
1.9-1.12 on amd64 and s390x
Fixes#32680.
Change-Id: I63e702c4c40602cb795ef71b1691eb704d38ccc7
Reviewed-on: https://go-review.googlesource.com/c/go/+/183059
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
For int8, int16, and int32, comparing their unsigned value to MaxInt64
to determine non-negativity doesn't make sense, because they have
negative values whose unsigned representation is smaller than that.
Fix is simply to compare with the appropriate upper bound based on the
value type's size.
Fixes#32560.
Change-Id: Ie7afad7a56af92bd890ba5ff33c86d1df06cfd9a
Reviewed-on: https://go-review.googlesource.com/c/go/+/181797
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This reverts CL 180761
Reason for revert: Reinstate the stack-allocated defer CL.
There was nothing wrong with the CL proper, but stack allocation of defers exposed two other issues.
Issue #32477: Fix has been submitted as CL 181258.
Issue #32498: Possible fix is CL 181377 (not submitted yet).
Change-Id: I32b3365d5026600069291b068bbba6cb15295eb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/181378
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The logic for detecting deferreturn calls is wrong.
We used to look for a relocation whose symbol is runtime.deferreturn
and has an offset of 0. But on some architectures, the relocation
offset is not zero. These include arm (the offset is 0xebfffffe) and
s390x (the offset is 6).
This ends up setting the deferreturn offset at 0, so we end up using
the entry point live map instead of the deferreturn live map in a
frame which defers and then segfaults.
Instead, use the IsDirectCall helper to find calls.
Fixes#32477
Update #6980
Change-Id: Iecb530a7cf6eabd7233be7d0731ffa78873f3a54
Reviewed-on: https://go-review.googlesource.com/c/go/+/181258
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Shrinks the size of things that can be stack allocated from
10M to 128k for declared variables and from 64k to 16k for
implicit allocations (new(T), &T{}, etc).
Usage: "go build -gcflags -smallframes hello.go"
An earlier GOEXPERIMENT version of this caused only one
problem, when a gc-should-detect-oversize-stack test no
longer had an oversized stack to detect. The change was
converted to a flag to make it easier to access (for
diagnosing "long" GC-related single-thread pauses) and to
remove interference with the test.
Includes test to verify behavior.
Updates #27732.
Change-Id: I1255d484331e77185e07c78389a8b594041204c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/180817
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This reverts commit fff4f599fe.
Reason for revert: Seems to still have issues around GC.
Fixes#32452
Change-Id: Ibe7af629f9ad6a3d5312acd7b066123f484da7f0
Reviewed-on: https://go-review.googlesource.com/c/go/+/180761
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
When a defer is executed at most once in a function body,
we can allocate the defer record for it on the stack instead
of on the heap.
This should make defers like this (which are very common) faster.
This optimization applies to 363 out of the 370 static defer sites
in the cmd/go binary.
name old time/op new time/op delta
Defer-4 52.2ns ± 5% 36.2ns ± 3% -30.70% (p=0.000 n=10+10)
Fixes#6980
Update #14939
Change-Id: I697109dd7aeef9e97a9eeba2ef65ff53d3ee1004
Reviewed-on: https://go-review.googlesource.com/c/go/+/171758
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Normally, reflect.makeFuncStub records the context value at a known
point in the stack frame, so that the runtime can get the argument map
for reflect.makeFuncStub from that known location.
This doesn't work for defers or goroutines that haven't started yet,
because they haven't allocated a frame or run an instruction yet. The
argument map must be extracted from the context value. We already do
this for defers (the non-nil ctxt arg to getArgInfo), we just need to
do it for unstarted goroutines as well.
When we traceback a goroutine, remember the context value from
g.sched. Use it for the first frame we find.
(We never need it for deeper frames, because we normally don't stop at
the start of reflect.makeFuncStub, as it is nosplit. With this CL we
could allow makeFuncStub to no longer be nosplit.)
Fixes#25897
Change-Id: I427abf332a741a80728cdc0b8412aa8f37e7c418
Reviewed-on: https://go-review.googlesource.com/c/go/+/180258
Reviewed-by: Cherry Zhang <cherryyz@google.com>
We need to make sure that there's no possible faulting
instruction between a VarDef and that variable being
fully initialized. If there was, then anything scanning
the stack during the handling of that fault will see
a live but uninitialized variable on the stack.
If we have:
NilCheck p
VarDef x
x = *p
We can't rewrite that to
VarDef x
NilCheck p
x = *p
Particularly, even though *p faults on p==nil, we still
have to do the explicit nil check before the VarDef.
Fixes#32288
Change-Id: Ib8b88e6a5af3bf6f238ff5491ac86f53f3cf9fc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/179239
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
The gccgo compiler crashes with int-to-string conversion with
large integer constant operand. CL 179777 is the fix. This CL
adds a test.
Updates #32347.
Change-Id: Id1d9dbbcdd3addca4636f1b9c5fdbc450cc48c1d
Reviewed-on: https://go-review.googlesource.com/c/go/+/179797
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This CL rewrites cmd/compile's package-level initialization ordering
algorithm to be compliant with the Go spec. See documentation in
initorder.go for details.
Incidentally, this CL also improves fidelity of initialization loop
diagnostics by including referenced functions in the emitted output
like go/types does.
Fixes#22326.
Change-Id: I7c9ac47ff563df4d4f700cf6195387a0f372cc7b
Reviewed-on: https://go-review.googlesource.com/c/go/+/170062
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
The code in #29218 resulted in an If block containing only its control.
That block was then converted by fuseIf into a plain block;
as a result, that control value was dead.
However, the control value was still present in b.Values.
This prevented further fusing of that block.
This change beefs up the check in fuseIf to allow fusing
blocks that contain only dead values (if any).
In the case of #29218, this enables enough extra
fusing that the control value could be eliminated,
allowing all values in turn to be eliminated.
This change also fuses 34 new blocks during make.bash.
It is not clear that this fixes every variant of #29218,
but it is a reasonable standalone change.
And code like #29218 is rare and fundamentally buggy,
so we can handle new instances if/when they actually occur.
Fixes#29218
Negligible toolspeed impact.
name old time/op new time/op delta
Template 213ms ± 3% 213ms ± 2% ~ (p=0.914 n=97+88)
Unicode 89.8ms ± 2% 89.6ms ± 2% -0.22% (p=0.045 n=93+95)
GoTypes 712ms ± 3% 709ms ± 2% -0.35% (p=0.023 n=95+95)
Compiler 3.24s ± 2% 3.23s ± 2% -0.30% (p=0.020 n=98+97)
SSA 10.0s ± 1% 10.0s ± 1% ~ (p=0.382 n=98+99)
Flate 135ms ± 3% 135ms ± 2% ~ (p=0.983 n=98+98)
GoParser 158ms ± 2% 158ms ± 2% ~ (p=0.170 n=99+99)
Reflect 447ms ± 3% 447ms ± 2% ~ (p=0.538 n=98+89)
Tar 189ms ± 2% 189ms ± 3% ~ (p=0.874 n=95+96)
XML 251ms ± 2% 251ms ± 2% ~ (p=0.434 n=94+96)
[Geo mean] 427ms 426ms -0.15%
name old user-time/op new user-time/op delta
Template 264ms ± 2% 265ms ± 2% ~ (p=0.075 n=96+90)
Unicode 119ms ± 6% 119ms ± 7% ~ (p=0.864 n=99+98)
GoTypes 926ms ± 2% 924ms ± 2% ~ (p=0.071 n=94+94)
Compiler 4.38s ± 2% 4.37s ± 2% -0.34% (p=0.001 n=98+97)
SSA 13.4s ± 1% 13.4s ± 1% ~ (p=0.693 n=90+93)
Flate 162ms ± 3% 161ms ± 2% ~ (p=0.163 n=99+99)
GoParser 186ms ± 2% 186ms ± 3% ~ (p=0.130 n=96+100)
Reflect 572ms ± 3% 572ms ± 2% ~ (p=0.608 n=97+97)
Tar 239ms ± 2% 239ms ± 3% ~ (p=0.999 n=93+91)
XML 302ms ± 2% 302ms ± 2% ~ (p=0.627 n=91+97)
[Geo mean] 540ms 540ms -0.08%
file before after Δ %
asm 4862704 4858608 -4096 -0.084%
compile 24001568 24001680 +112 +0.000%
total 132520780 132516796 -3984 -0.003%
file before after Δ %
cmd/compile/internal/gc.a 8887638 8887596 -42 -0.000%
cmd/compile/internal/ssa.a 29995056 29998986 +3930 +0.013%
cmd/internal/obj/wasm.a 209444 203652 -5792 -2.765%
total 129471798 129469894 -1904 -0.001%
Change-Id: I2d18f9278e68b9766058ae8ca621e844f9d89dd8
Reviewed-on: https://go-review.googlesource.com/c/go/+/177140
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This test was designed for #15609 and didn't consider nacl. It's not
worth adding new +build-guarded assembly files in issue15609.dir for
nacl, especially as nacl is going away.
Fixes#32206
Change-Id: Ic5bd48b4f790a1f7019100b8a72d4688df75512f
Reviewed-on: https://go-review.googlesource.com/c/go/+/178698
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Currently, some tests under test/fixedbugs never run:
$ for d in test/fixedbugs/*.dir; do
! test -f "${d%.dir}.go" && echo "$d"
done
test/fixedbugs/issue15071.dir
test/fixedbugs/issue15609.dir
test/fixedbugs/issue29612.dir
Because they missed the corresponding ".go" file, so "go run run.go"
will skip them.
Add missing ".go" files for those tests to make sure they will be
collected and run.
While at it, add another action "runindir", which does "go run ."
inside the t.goDirName then check the output.
Change-Id: I88000b3663a6a615d90c1cf11844ea0377403e3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/177798
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
As an optimization, function literals capture variables by value when
they're not assigned and their address has not been taken. Because
result parameters are implicitly assigned through return statements
(which do not otherwise set the "assigned" flag), result parameters
are explicitly handled to always capture by reference.
However, the logic was slightly mistaken because it was only checking
if the variable in the immediately enclosing context was a return
parameter, whereas in a multiply-nested function literal it would
itself be another closure variable (PAUTOHEAP) rather than a return
parameter (PPARAMOUT).
The fix is to simply test the outermost variable, like the rest of the
if statement's tests were already doing.
Fixes#32175.
Change-Id: Ibadde033ff89a1b47584b3f56c0014d8e5a74512
Reviewed-on: https://go-review.googlesource.com/c/go/+/178541
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
First, remove the randomization of initialization order.
Then, revert to source code order instead of sorted package path order.
This restores the behavior that was in 1.12.
A larger change which will implement the suggestion in #31636 will
wait for 1.14. It's too complicated for 1.13 at this point (it has
tricky interactions with plugins).
Fixes#31636
Change-Id: I35b48e8cc21cf9f93c0973edd9193d2eac197628
Reviewed-on: https://go-review.googlesource.com/c/go/+/178297
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
typecheck type alias always replaces the original definition of the symbol.
This is wrong behavior because if the symbol's definition is replaced by a
local type alias, it ends up being written to compiled file as an alias,
instead of the original type.
To fix, only replace the definition of symbol with global type alias.
Fixes#31959
Change-Id: Id85a15e8a9d6a4b06727e655a95dc81e63df633a
Reviewed-on: https://go-review.googlesource.com/c/go/+/177378
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The test/codegen tests check all architectures
mentioned in the test file, but this requires
building at least the runtime for that architecture.
This CL changes the test to only check the local
architecture, leaving checking of other architectures
to the relevant builders, as usual.
This cuts 'go run run.go codegen' by 12r 78u 21s.
After this change, all.bash runs in ~4:40 on my laptop.
For #26473.
Change-Id: Ia0354d1aff2df2949f838528c8171410bc42dc8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/177577
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The CL 164718 mistyped the comparison flags. The rules for floating
point comparison should be GreaterThanF and GreaterEqualF. Fortunately,
the wrong optimizations were overwritten by other integer rules, so the
issue won't cause failure but just some performance impact.
The fixed CL optimizes the floating point test as follows.
source code: func foo(f float64) bool { return f > 4 || f < -4}
previous version: "FCMPD", "CSET\tGT", "CBZ"
fixed version: "FCMPD", BLE"
Add the test case.
Change-Id: Iea954fdbb8272b2d642dae0f816dc77286e6e1fa
Reviewed-on: https://go-review.googlesource.com/c/go/+/177121
Reviewed-by: Ben Shi <powerman1st@163.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
If a slice's entries are sparse, we decide to initialize it dynamically
instead of statically. That's CL 151319.
But if we do initialize it dynamically, we still need to initialize
the static entries. Typically we do that, but the bug fixed here is
that we don't if the entry's value is itself an array or struct.
To fix, use initKindLocalCode to ensure that both static and
dynamic entries are initialized via code.
Fixes#31987
Change-Id: I1192ffdbfb5cd50445c1206c4a3d8253295201dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/176904
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
MOVBstore's value argument is a value, not a flag. We are storing
a byte so just use UInt8.
Fixes#31915.
Change-Id: Id799e5f44efc3a9c3d8480f6f25ad032c2a631bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/176719
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Adjust the dummy use function to a real use. As suggested by the
println calls in the test, nilptr2.go supposes to check that a
used nil pointer dereference panics. This use function is not
real enough so an optimized compiler such as gccgo could
eliminate the call.
The spec requires that even a dummy use would cause a panic.
Unfortunately, due to #31151 this is not true for gccgo at -O1 or
above.
Change-Id: Ie07c8a5969ab94dad82d4f7cfec30597c25b7c46
Reviewed-on: https://go-review.googlesource.com/c/go/+/176579
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change is a simple work-around to avoid a compiler crash
and provide a reasonable error message. A future change should
fix the root cause for this problem.
Fixes#23823.
Change-Id: Ifc80d9f4d35e063c378e54d5cd8d1cf4c0d2ec6a
Reviewed-on: https://go-review.googlesource.com/c/go/+/175518
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Before this CL we used to panic with "nil pointer dereference" because
the value we're calling assignTo on is the zero Value. Provide a better
error message.
Fixes#28748
Change-Id: I7dd4c9e30b599863664d91e78cc45878d8b0052e
Reviewed-on: https://go-review.googlesource.com/c/go/+/175440
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This CL adds intrinsics for the 64-bit addition and subtraction
functions in math/bits. These intrinsics use the condition code
to propagate the carry or borrow bit.
To make the carry chains more efficient I've removed the
'clobberFlags' property from most of the load and store
operations. Originally these ops did clobber flags when using
offsets that didn't fit in a signed 20-bit integer, however
that is no longer true.
As with other platforms the intrinsics are faster when executed
in a chain rather than a loop because currently we need to spill
and restore the carry bit between each loop iteration. We may
be able to reduce the need to do this on s390x (e.g. by using
compare-and-branch instructions that do not clobber flags) in the
future.
name old time/op new time/op delta
Add64 1.21ns ± 2% 2.03ns ± 2% +67.18% (p=0.000 n=7+10)
Add64multiple 2.98ns ± 3% 1.03ns ± 0% -65.39% (p=0.000 n=10+9)
Sub64 1.23ns ± 4% 2.03ns ± 1% +64.85% (p=0.000 n=10+10)
Sub64multiple 3.73ns ± 4% 1.04ns ± 1% -72.28% (p=0.000 n=10+8)
Change-Id: I913bbd5e19e6b95bef52f5bc4f14d6fe40119083
Reviewed-on: https://go-review.googlesource.com/c/go/+/174303
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
golang.org/cl/174498 add ONAME case to isStaticCompositeLiteral, to
detect global variable as compile-time constant.
It does report wrong for struct field, e.g:
o := one{i: two{i: 42}.i}
field i in two{i: 42} was reported as static composite literal, while it
should not.
In general, adding ONAME case for isStaticCompositeLiteral is probably
wrong.
Fixes#31782
Change-Id: Icde7d43bbb002b75df5c52b948b7126a4265e07b
Reviewed-on: https://go-review.googlesource.com/c/go/+/174837
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
golang.org/cl/174498 removes dynamic map entry handling in maplit, by
filtering the static entry only. It panics if it see a dynamic entry.
It relies on order to remove all dynamic entries.
But after recursively call order on the statics, some static entries
become dynamic, e.g OCONVIFACE node:
type i interface {
j()
}
type s struct{}
func (s) j() {}
type foo map[string]i
var f = foo{
"1": s{},
}
To fix it, we recursively call order on each static entry, if it changed
to dynamic, put entry to dynamic then.
Fixes#31777
Change-Id: I1004190ac8f2d1eaa4beb6beab989db74099b025
Reviewed-on: https://go-review.googlesource.com/c/go/+/174777
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This enables more of the testcases in memcombine for ppc64le,
and adds more detail to some existing.
Change-Id: Ic522a1175bed682b546909c96f9ea758f8db247c
Reviewed-on: https://go-review.googlesource.com/c/go/+/174737
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
"Division by invariant integers using multiplication" paper
by Granlund and Montgomery contains a method for directly computing
divisibility (x%c == 0 for c constant) by means of the modular inverse.
The method is further elaborated in "Hacker's Delight" by Warren Section 10-17
This general rule can compute divisibilty by one multiplication, and add
and a compare for odd divisors and an additional rotate for even divisors.
To apply the divisibility rule, we must take into account
the rules to rewrite x%c = x-((x/c)*c) and (x/c) for c constant on the first
optimization pass "opt". This complicates the matching as we want to match
only in the cases where the result of (x/c) is not also needed.
So, we must match on the expanded form of (x/c) in the expression x == c*(x/c)
in the "late opt" pass after common subexpresion elimination.
Note, that if there is an intermediate opt pass introduced in the future we
could simplify these rules by delaying the magic division rewrite to "late opt"
and matching directly on (x/c) in the intermediate opt pass.
On amd64, the divisibility check is 30-45% faster.
name old time/op new time/op delta`
DivisiblePow2constI64-4 0.83ns ± 1% 0.82ns ± 0% ~ (p=0.079 n=5+4)
DivisibleconstI64-4 2.68ns ± 1% 1.87ns ± 0% -30.33% (p=0.000 n=5+4)
DivisibleWDivconstI64-4 2.69ns ± 1% 2.71ns ± 3% ~ (p=1.000 n=5+5)
DivisiblePow2constI32-4 1.15ns ± 1% 1.15ns ± 0% ~ (p=0.238 n=5+4)
DivisibleconstI32-4 2.24ns ± 1% 1.20ns ± 0% -46.48% (p=0.016 n=5+4)
DivisibleWDivconstI32-4 2.27ns ± 1% 2.27ns ± 1% ~ (p=0.683 n=5+5)
DivisiblePow2constI16-4 0.81ns ± 1% 0.82ns ± 1% ~ (p=0.135 n=5+5)
DivisibleconstI16-4 2.11ns ± 2% 1.20ns ± 1% -42.99% (p=0.008 n=5+5)
DivisibleWDivconstI16-4 2.23ns ± 0% 2.27ns ± 2% +1.79% (p=0.029 n=4+4)
DivisiblePow2constI8-4 0.81ns ± 1% 0.81ns ± 1% ~ (p=0.286 n=5+5)
DivisibleconstI8-4 2.13ns ± 3% 1.19ns ± 1% -43.84% (p=0.008 n=5+5)
DivisibleWDivconstI8-4 2.23ns ± 1% 2.25ns ± 1% ~ (p=0.183 n=5+5)
Fixes#30282Fixes#15806
Change-Id: Id20d78263a4fdfe0509229ae4dfa2fede83fc1d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/173998
Run-TryBot: Brian Kessler <brian.m.kessler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
In the statement x = a[i], the index panic should appear to come from
the line number of the '['. Previous to this CL we sometimes used the
line number of the '=' instead.
Fixes#29504
Change-Id: Ie718fd303c1ac2aee33e88d52c9ba9bcf220dea1
Reviewed-on: https://go-review.googlesource.com/c/go/+/174617
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Any code that imports the testing package forces the testing flags to be
defined, even in non-test binaries. People work around this today by
defining a copy of the testing.TB interface just to avoid importing
testing.
Fix this by moving flag registration into a new function, testing.Init.
Delay calling Init until the testing binary begins to run, in
testing.MainStart.
Init is exported for cases where users need the testing flags to be
defined outside of a "go test" context. In particular, this may be
needed where testing.Benchmark is called outside of a test.
Fixes#21051
Change-Id: Ib7e02459e693c26ae1ba71bbae7d455a91118ee3
Reviewed-on: https://go-review.googlesource.com/c/go/+/173722
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change creates an intrinsic for Add64 for ppc64x and adds a
testcase for it.
name old time/op new time/op delta
Add64-160 1.90ns ±40% 2.29ns ± 0% ~ (p=0.119 n=5+5)
Add64multiple-160 6.69ns ± 2% 2.45ns ± 4% -63.47% (p=0.016 n=4+5)
Change-Id: I9abe6fb023fdf62eea3c9b46a1820f60bb0a7f97
Reviewed-on: https://go-review.googlesource.com/c/go/+/173758
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
Run-TryBot: Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
"Division by invariant integers using multiplication" paper
by Granlund and Montgomery contains a method for directly computing
divisibility (x%c == 0 for c constant) by means of the modular inverse.
The method is further elaborated in "Hacker's Delight" by Warren Section 10-17
This general rule can compute divisibilty by one multiplication and a compare
for odd divisors and an additional rotate for even divisors.
To apply the divisibility rule, we must take into account
the rules to rewrite x%c = x-((x/c)*c) and (x/c) for c constant on the first
optimization pass "opt". This complicates the matching as we want to match
only in the cases where the result of (x/c) is not also available.
So, we must match on the expanded form of (x/c) in the expression x == c*(x/c)
in the "late opt" pass after common subexpresion elimination.
Note, that if there is an intermediate opt pass introduced in the future we
could simplify these rules by delaying the magic division rewrite to "late opt"
and matching directly on (x/c) in the intermediate opt pass.
Additional rules to lower the generic RotateLeft* ops were also applied.
On amd64, the divisibility check is 25-50% faster.
name old time/op new time/op delta
DivconstI64-4 2.08ns ± 0% 2.08ns ± 1% ~ (p=0.881 n=5+5)
DivisibleconstI64-4 2.67ns ± 0% 2.67ns ± 1% ~ (p=1.000 n=5+5)
DivisibleWDivconstI64-4 2.67ns ± 0% 2.67ns ± 0% ~ (p=0.683 n=5+5)
DivconstU64-4 2.08ns ± 1% 2.08ns ± 1% ~ (p=1.000 n=5+5)
DivisibleconstU64-4 2.77ns ± 1% 1.55ns ± 2% -43.90% (p=0.008 n=5+5)
DivisibleWDivconstU64-4 2.99ns ± 1% 2.99ns ± 1% ~ (p=1.000 n=5+5)
DivconstI32-4 1.53ns ± 2% 1.53ns ± 0% ~ (p=1.000 n=5+5)
DivisibleconstI32-4 2.23ns ± 0% 2.25ns ± 3% ~ (p=0.167 n=5+5)
DivisibleWDivconstI32-4 2.27ns ± 1% 2.27ns ± 1% ~ (p=0.429 n=5+5)
DivconstU32-4 1.78ns ± 0% 1.78ns ± 1% ~ (p=1.000 n=4+5)
DivisibleconstU32-4 2.52ns ± 2% 1.26ns ± 0% -49.96% (p=0.000 n=5+4)
DivisibleWDivconstU32-4 2.63ns ± 0% 2.85ns ±10% +8.29% (p=0.016 n=4+5)
DivconstI16-4 1.54ns ± 0% 1.54ns ± 0% ~ (p=0.333 n=4+5)
DivisibleconstI16-4 2.10ns ± 0% 2.10ns ± 1% ~ (p=0.571 n=4+5)
DivisibleWDivconstI16-4 2.22ns ± 0% 2.23ns ± 1% ~ (p=0.556 n=4+5)
DivconstU16-4 1.09ns ± 0% 1.01ns ± 1% -7.74% (p=0.000 n=4+5)
DivisibleconstU16-4 1.83ns ± 0% 1.26ns ± 0% -31.52% (p=0.008 n=5+5)
DivisibleWDivconstU16-4 1.88ns ± 0% 1.89ns ± 1% ~ (p=0.365 n=5+5)
DivconstI8-4 1.54ns ± 1% 1.54ns ± 1% ~ (p=1.000 n=5+5)
DivisibleconstI8-4 2.10ns ± 0% 2.11ns ± 0% ~ (p=0.238 n=5+4)
DivisibleWDivconstI8-4 2.22ns ± 0% 2.23ns ± 2% ~ (p=0.762 n=5+5)
DivconstU8-4 0.92ns ± 1% 0.94ns ± 1% +2.65% (p=0.008 n=5+5)
DivisibleconstU8-4 1.66ns ± 0% 1.26ns ± 1% -24.28% (p=0.008 n=5+5)
DivisibleWDivconstU8-4 1.79ns ± 0% 1.80ns ± 1% ~ (p=0.079 n=4+5)
A follow-up change will address the signed division case.
Updates #30282
Change-Id: I7e995f167179aa5c76bb10fbcbeb49c520943403
Reviewed-on: https://go-review.googlesource.com/c/go/+/168037
Run-TryBot: Brian Kessler <brian.m.kessler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
For powers of two (c=1<<k), the divisibility check x%c == 0 can be made
just by checking the trailing zeroes via a mask x&(c-1) == 0 even for signed
integers. This avoids division fix-ups when just divisibility check is needed.
To apply this rule, we match on the fixed-up version of the division. This is
neccessary because the mod and division rewrite rules are already applied
during the initial opt pass.
The speed up on amd64 due to elimination of unneccessary fix-up code is ~55%:
name old time/op new time/op delta
DivconstI64-4 2.08ns ± 0% 2.09ns ± 1% ~ (p=0.730 n=5+5)
DivisiblePow2constI64-4 1.78ns ± 1% 0.81ns ± 1% -54.66% (p=0.008 n=5+5)
DivconstU64-4 2.08ns ± 0% 2.08ns ± 0% ~ (p=0.683 n=5+5)
DivconstI32-4 1.53ns ± 0% 1.53ns ± 1% ~ (p=0.968 n=4+5)
DivisiblePow2constI32-4 1.79ns ± 1% 0.81ns ± 1% -54.97% (p=0.008 n=5+5)
DivconstU32-4 1.78ns ± 1% 1.80ns ± 2% ~ (p=0.206 n=5+5)
DivconstI16-4 1.54ns ± 2% 1.54ns ± 0% ~ (p=0.238 n=5+4)
DivisiblePow2constI16-4 1.78ns ± 0% 0.81ns ± 1% -54.72% (p=0.000 n=4+5)
DivconstU16-4 1.00ns ± 5% 1.01ns ± 1% ~ (p=0.119 n=5+5)
DivconstI8-4 1.54ns ± 0% 1.54ns ± 2% ~ (p=0.571 n=4+5)
DivisiblePow2constI8-4 1.78ns ± 0% 0.82ns ± 8% -53.71% (p=0.008 n=5+5)
DivconstU8-4 0.93ns ± 1% 0.93ns ± 1% ~ (p=0.643 n=5+5)
A follow-up CL will address the general case of x%c == 0 for signed integers.
Updates #15806
Change-Id: Iabadbbe369b6e0998c8ce85d038ebc236142e42a
Reviewed-on: https://go-review.googlesource.com/c/go/+/173557
Run-TryBot: Brian Kessler <brian.m.kessler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
We already skipped blank field initialization in non-global contexts.
This change makes the global context treatment match.
Fixes#31546
Change-Id: I40acce49b0a9deb351ae0da098f4c114e425ec63
Reviewed-on: https://go-review.googlesource.com/c/go/+/173723
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This pair of packages caused a crash in gollvm, due to a glitch in the
way the front end handles empty/non-name parameters for functions that
are inline candidates.
Updates #31637.
Change-Id: I571c0658a00974dd36025e571638c0c836a3cdfd
Reviewed-on: https://go-review.googlesource.com/c/go/+/173617
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This reverts CL 168038 (git 68819fb6d2)
Reason for revert: Doesn't work on 32 bit archs.
Change-Id: Idec9098060dc65bc2f774c5383f0477f8eb63a3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/173442
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
For powers of two (c=1<<k), the divisibility check x%c == 0 can be made
just by checking the trailing zeroes via a mask x&(c-1)==0 even for signed
integers. This avoids division fixups when just divisibility check is needed.
To apply this rule the generic divisibility rule for A%B = A-(A/B*B) is disabled
on the "opt" pass, but this does not affect generated code as this rule is applied
later.
The speed up on amd64 due to elimination of unneccessary fixup code is ~55%:
name old time/op new time/op delta
DivconstI64-4 2.08ns ± 0% 2.07ns ± 0% ~ (p=0.079 n=5+5)
DivisiblePow2constI64-4 1.78ns ± 1% 0.81ns ± 1% -54.55% (p=0.008 n=5+5)
DivconstU64-4 2.08ns ± 0% 2.08ns ± 0% ~ (p=1.000 n=5+5)
DivconstI32-4 1.53ns ± 0% 1.53ns ± 0% ~ (all equal)
DivisiblePow2constI32-4 1.79ns ± 1% 0.81ns ± 4% -54.75% (p=0.008 n=5+5)
DivconstU32-4 1.78ns ± 1% 1.78ns ± 1% ~ (p=1.000 n=5+5)
DivconstI16-4 1.54ns ± 2% 1.53ns ± 0% ~ (p=0.333 n=5+4)
DivisiblePow2constI16-4 1.78ns ± 0% 0.79ns ± 1% -55.39% (p=0.000 n=4+5)
DivconstU16-4 1.00ns ± 5% 0.99ns ± 1% ~ (p=0.730 n=5+5)
DivconstI8-4 1.54ns ± 0% 1.53ns ± 0% ~ (p=0.714 n=4+5)
DivisiblePow2constI8-4 1.78ns ± 0% 0.80ns ± 0% -55.06% (p=0.000 n=5+4)
DivconstU8-4 0.93ns ± 1% 0.95ns ± 1% +1.72% (p=0.024 n=5+5)
A follow-up CL will address the general case of x%c == 0 for signed integers.
Updates #15806
Change-Id: I0d284863774b1bc8c4ce87443bbaec6103e14ef4
Reviewed-on: https://go-review.googlesource.com/c/go/+/168038
Reviewed-by: Keith Randall <khr@golang.org>
In 31618, we end up comparing the is-stmt-ness of positions
to repurpose real instructions as inline marks. If the is-stmt-ness
doesn't match, we end up not being able to remove the inline mark.
Always use statement-full positions to do the matching, so we
always find a match if there is one.
Also always use positions that are statements for inline marks.
Fixes#31618
Change-Id: Idaf39bdb32fa45238d5cd52973cadf4504f947d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/173324
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Stack object generation code was always using the local package name
for its symbol. Normally that doesn't matter, as we usually only
compile functions in the local package. But for wrappers, the compiler
generates functions which live in other packages. When there are two
other packages with identical functions to wrap, the same name appears
twice, and the compiler goes boom.
Fixes#31252
Change-Id: I7026eebabe562cb159b8b6046cf656afd336ba25
Reviewed-on: https://go-review.googlesource.com/c/go/+/171464
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The special case logic for go/defer arguments in Escape.call was
scattered around a bit and was somewhat inconsistently handled across
different types of function calls and parameters. This CL pulls the
logic out into a separate callStmt method that's used uniformly for
all kinds of function calls and arguments.
Fixes#31573.
Change-Id: Icdcdf611754dc3fcf1af7cb52879fb4b73a7a31f
Reviewed-on: https://go-review.googlesource.com/c/go/+/173019
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
A check in inl.go to prevent inlining of functions calling
either getcallerpc or getcallersp does not work when these
functions are intrinsics. Swap checks to fix.
Includes test.
No bug, this was discovered in the course of a ridiculous
experiment with inlining.
Change-Id: Ie1392523bb89882d586678f2674e1a4eadc5e431
Reviewed-on: https://go-review.googlesource.com/c/go/+/172217
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Casp1 is implemented in Go on js/wasm, so escape analysis correctly
determines that the "old" parameter does not escape (which is good).
Unfortunately, test/run.go doesn't have a way to indicate that ERROR
messages are optional, and cmd/compile only emits diagnostics for "var
x int" when it's moved to the heap; not when it stays on the stack.
To accomodate that this test currently passes on some GOARCHes but not
others, rewrite the Casp1 test to use "x := new(int)" and allow both
"new(int) escapes to heap" or "new(int) does not escape".
Updates #31525.
Change-Id: I40150a7ff9042f184386ccdb2d4d428f63e8ba4f
Reviewed-on: https://go-review.googlesource.com/c/go/+/172602
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The //go:noescape directive says that arguments don't leak at all,
which is too aggressive of a claim for functions that return pointers
derived from their parameters.
Remove the directive for now. Long term fix will require a new
directive that allows more fine-grained control over escape analysis
information supplied for functions implemented in assembly.
Also, update the BAD comments in the test cases for Loadp: we really
want that *ptr leaks to the result parameter, not that *ptr leaks to
the heap.
Updates #31525.
Change-Id: Ibfa61f2b70daa7ed3223056b57eeee777eef2e31
Reviewed-on: https://go-review.googlesource.com/c/go/+/172578
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Cherry pointed out this case in review for CL 136496. That CL was
slightly too aggressive, and I likely would have made the same mistake
if I tried it myself.
Updates #27772.
Change-Id: I1fafabb9f8d9aba0494aa71333a4e17cf1bac5c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/172421
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
There weren't any tests to make sure these work correctly, and this
led to escape analysis regressions in both linux/s390x and js/wasm.
The underlying issue that cmd/compile is only getting some of these
correct because escape analysis doesn't understand //go:linkname is
still present, but at least this addresses the fragility aspect.
Updates #15283.
Change-Id: I546aee1899d098b2e3de45e9b33c3ca22de485f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/172420
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
In typecheckclosure, a xfunc node will be put to xtop. But that node can
be shared between multiple closures, like in a const declaration group:
const (
x = unsafe.Sizeof(func() {})
y
)
It makes a xfunc node appears multiple times in xtop, causing duplicate
initLSym run.
To fix this issue, we only do typecheck for xfunc one time, and setup
closure node earlier in typecheckclosure process.
Fixes#30709
Change-Id: Ic924a157ee9f3e5d776214bef5390849ddc8aab9
Reviewed-on: https://go-review.googlesource.com/c/go/+/172298
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The new escape analysis implementation tries to emit debugging
diagnostics that are compatible with the existing implementation, but
there's a handful of cases that are easier to handle by updating the
test expectations instead.
For regress tests that need updating, the original file is copied to
oldescapeXXX.go.go with -newescape=false added to the //errorcheck
line, while the file is updated in place with -newescape=true and new
test requirements.
Notable test changes:
1) escape_because.go looks for a lot of detailed internal debugging
messages that are fairly particular to how esc.go works and that I
haven't attempted to port over to escape.go yet.
2) There are a lot of "leaking param: x to result ~r1 level=-1"
messages for code like
func(p *int) *T { return &T{p} }
that were simply wrong. Here &T must be heap allocated unconditionally
(because it's being returned); and since p is stored into it, p
escapes unconditionally too. esc.go incorrectly reports that p escapes
conditionally only if the returned pointer escaped.
3) esc.go used to print each "leaking param" analysis result as it
discovered them, which could lead to redundant messages (e.g., that a
param leaks at level=0 and level=1). escape.go instead prints
everything at the end, once it knows the shortest path to each sink.
4) esc.go didn't precisely model direct-interface types, resulting in
some values unnecessarily escaping to the heap when stored into
non-escaping interface values.
5) For functions written in assembly, esc.go only printed "does not
escape" messages, whereas escape.go prints "does not escape" or
"leaking param" as appropriate, consistent with the behavior for
functions written in Go.
6) 12 tests included "BAD" annotations identifying cases where esc.go
was unnecessarily heap allocating something. These are all fixed by
escape.go.
Updates #23109.
Change-Id: Iabc9eb14c94c9cadde3b183478d1fd54f013502f
Reviewed-on: https://go-review.googlesource.com/c/go/+/170447
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
For a failed interface conversion not in ",ok" form, getitab
calls itab.init to get the name of the missing method for the
panic message. itab.init will try to find the methods, populate
the method table as it goes. When some method is missing, it sets
itab.fun[0] to 0 before return. There is a small window that
itab.fun[0] could be non-zero.
If concurrently, another goroutine tries to do the same interface
conversion, it will read the same itab's fun[0]. If this happens
in the small window, it sees a non-zero fun[0] and thinks the
conversion succeeded, which is bad.
Fix the race by setting fun[0] to non-zero only when we know the
conversion succeeds. While here, also simplify the syntax
slightly.
Fixes#31419.
Change-Id: Ied34d3043079eb933e330c5877b85e13f98f1916
Reviewed-on: https://go-review.googlesource.com/c/go/+/171759
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
These new calls should not prevent NOSPLIT promotion, like the old ones.
These new calls should not prevent racefuncenter/exit removal.
(The latter was already true, as the new calls are not yet lowered
to StaticCalls at the point where racefuncenter/exit removal is done.)
Add tests to make sure we don't regress (again).
Fixes#31219
Change-Id: I3fb6b17cdd32c425829f1e2498defa813a5a9ace
Reviewed-on: https://go-review.googlesource.com/c/go/+/170639
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
Some var declarations return "extra expression" or "missing expression"
errors when they should return “assignment mismatch” instead. Change
the returned error messages to exhibit the desired behavior.
Fixes#30085.
Change-Id: I7189355fbb0f976d70100779db4f81a9ae64fb11
Reviewed-on: https://go-review.googlesource.com/c/go/+/161558
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
"leaking closure reference" is redundant for similar reasons as "&x
escapes to heap" for OADDR nodes: the reference itself does not
allocate, and we already report when the referenced variable is moved
to heap.
"mark escaped content" is redundant with "leaking param content".
Updates #23109.
Change-Id: I1ab599cb1e8434f1918dd80596a70cba7dc8a0cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/170321
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
For most nodes (e.g., OPTRLIT, OMAKESLICE, OCONVIFACE), escape
analysis prints "escapes to heap" or "does not escape" to indicate
whether that node's allocation can be heap or stack allocated.
These messages are also emitted for OADDR, even though OADDR does not
actually allocate anything itself. Moreover, it's redundant because
escape analysis already prints "moved to heap" diagnostics when an
OADDR node like "&x" causes x to require heap allocation.
Because OADDR nodes don't allocate memory, my escape analysis rewrite
doesn't naturally emit the "escapes to heap" / "does not escape"
diagnostics for them. It's also non-trivial to replicate the exact
semantics esc.go uses for OADDR.
Since there are so many of these messages, I'm disabling them in this
CL by themselves. I modified esc.go to suppress the Warnl calls
without any other behavior changes, and then used a shell script to
automatically remove any ERROR messages mentioned by run.go in
"missing error" or "no match for" lines.
Fixes#16300.
Updates #23109.
Change-Id: I3993e2743c3ff83ccd0893f4e73b366ff8871a57
Reviewed-on: https://go-review.googlesource.com/c/go/+/170319
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
The bug in 29612 is that there are two similar-looking anonymous interface
types in two different packages, ./p1/ssa and ./p2/ssa:
v.(interface{ foo() }).foo()
These types should be treated differently because the unexported method
makes the types different (according to the spec).
But when generating the type descriptors for those two types, they
both have the name "interface { ssa.foo() }". They thus get the same
symbol, and the linker happily unifies them. It picks an arbitrary one
for the runtime to use, but that breaks conversions from concrete types
that have a foo method from the package which had its interface type
overwritten.
We need to encode the metadata symbol for unexported methods as package
path qualified (The same as we did in CL 27791 for struct fields).
So switching from FmtUnsigned to Fmtleft by default fixes the issue.
In case of generating namedata, FmtUnsigned is used.
The benchmark result ends up in no significant change of compiled binary
compare to the immediate parent.
Fixes#29612
Change-Id: I775aff91ae4a1bb16eb18a48d55e3b606f3f3352
Reviewed-on: https://go-review.googlesource.com/c/go/+/170157
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
When branching at a bounds check for indexing or slicing ops, prove currently
only learns from the upper bound. On the positive branch, we currently learn
i < len(a) (or i <= len(a)) in both the signed and unsigned domains.
This CL makes prove also learn from the lower bound. Specifically, on the
positive branch from index or slicing ops, prove will now ALSO learn i >= 0 in
the signed domain (this fact is of no value in the unsigned domain).
The substantive change itself is only an additional call to addRestrictions,
though I've also inverted the nested switch statements around that call for the
sake of clarity.
This CL removes 92 bounds checks from std and cmd. It passes all tests and
shows no deltas on compilecmp.
Fixes#28885
Change-Id: I13eccc36e640eb599fa6dc5aa3be3c7d7abd2d9e
Reviewed-on: https://go-review.googlesource.com/c/go/+/170121
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Would suggest extending capabilities (32-bit, unsigned, etc)
in separate CLs because prove bugs are so mystifying.
This implements the suggestion in this comment
https://go-review.googlesource.com/c/go/+/104041/10/src/cmd/compile/internal/ssa/loopbce.go#164
for inferring properly bounded iteration for loops of the form
for i := K0; i < KNN-(K-1); i += K
for i := K0; i <= KNN-K; i += K
Where KNN is "known non negative" (i.e., len or cap) and K
is also not negative. Because i <= KNN-K, i+K <= KNN and
no overflow occurs.
Also handles decreasing case (K1 > 0)
for i := KNN; i >= K0; i -= K1
which works when MININT+K1 < K0
(i.e. MININT < K0-K1, no overflow)
Signed only, also only 64 bit for now.
Change-Id: I5da6015aba2f781ec76c4ad59c9c48d952325fdc
Reviewed-on: https://go-review.googlesource.com/c/go/+/136375
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexandru Moșoi <alexandru@mosoi.ro>
Prove currently fails to remove bounds checks of the form:
if i >= 0 { // hint that i is non-negative
for i < len(data) { // i becomes Phi in the loop SSA
_ = data[i] // data[Phi]; bounds check!!
i++
}
}
addIndVarRestrictions fails to identify that the loop induction
variable, (Phi), is non-negative. As a result, the restrictions,
i <= Phi < len(data), are only added for the signed domain. When
testing the bounds check, addBranchRestrictions is similarly unable
to infer that Phi is non-negative. As a result, the restriction,
Phi >= len(data), is only added/tested for the unsigned domain.
This CL changes the isNonNegative method to utilise the factTable's
partially ordered set (poset). It also adds field factTable.zero to
allow isNonNegative to query the poset using the zero(0) constant
found or created early in prove.
Fixes#28956
Change-Id: I792f886c652eeaa339b0d57d5faefbf5922fe44f
Reviewed-on: https://go-review.googlesource.com/c/go/+/161437
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Typechecking treats all untyped numbers as integers for the purposes
of validating operators. However, when I refactoring constant
operation evalution in golang.org/cl/139901, I mistakenly interpreted
that the only invalid case that needed to be preserved was % (modulo)
on floating-point values.
This CL restores the other remaining cases that were dropped from that
CL. It also uses the phrasing "invalid operation" instead of "illegal
constant expression" for better consistency with the rest of
cmd/compile and with go/types.
Lastly, this CL extends setconst to recognize failed constant folding
(e.g., division by zero) so that we can properly mark those
expressions as broken rather than continuing forward with bogus values
that might lead to further spurious errors.
Fixes#31060.
Change-Id: I1ab6491371925e22bc8b95649f1a0eed010abca6
Reviewed-on: https://go-review.googlesource.com/c/go/+/169719
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Update the issue 30908 test to work with the no-opt builder
(this requires a corresponding change in the linker as well).
As part of this change, 'rundir' tests are now linked without
passing "-w" to the linker.
Updates #30908.
Fixes#31034.
Change-Id: Ic776e1607075c295e409e1c8230aaf55a79a6323
Reviewed-on: https://go-review.googlesource.com/c/go/+/169161
Reviewed-by: Ian Lance Taylor <iant@golang.org>
CL 135377 introduces pass strings and slices to convT2{E,I} by value.
Before that CL, all types, except interface will be allocated temporary
address. The CL changes the logic that only constant and type which
needs address (determine by convFuncName) will be allocated.
It fails to cover the case where type is static composite literal.
Adding condition to check that case fixes the issue.
Also, static composite literal node implies constant type, so consttype
checking can be removed.
Fixes#30956
Change-Id: Ifc750a029fb4889c2d06e73e44bf85e6ef4ce881
Reviewed-on: https://go-review.googlesource.com/c/go/+/168858
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Some special-case code paths in order.go didn't expect OCALLFUNC to
have Ninit; in particular, OAS2FUNC and ODEFER/OGO failed to call
o.init on their child OCALLFUNC node. This resulted in not all of the
AST being properly ordered.
This was noticed because order is responsible for introducing an
invariant around how OAPPEND is used, which is enforced by walk.
However, there were perhaps simpler cases (e.g., simple order of
evaluation) that were being silently miscompiled.
Fixes#31010.
Change-Id: Ib928890ab5ec2aebd8e30a030bc2b404387f9123
Reviewed-on: https://go-review.googlesource.com/c/go/+/169257
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
New test case designed to mimic the code in issue 30908, which
features duplicate but non-indentical DWARF abstract subprogram DIEs.
Updates #30908.
Change-Id: Iacb4b53e6a988e46c801cdac236cef883c553f8f
Reviewed-on: https://go-review.googlesource.com/c/go/+/168957
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
For "rundir" tests, allow users to add in linker flags as well as
compiler flags, e.g.
// rundir -m -ldflags -w
The directive above will pass "-m" to the compiler on each package compilation
and "-w" to the linker for the final link.
In addition, if "-P" is specified with 'rundir', then for each compile
pass in "-p <X>" to set the packagepath explicitly, which is closer to
how the compiler is run by 'go build'.
Change-Id: I04720011a89d1bd8dcb4f2ccb4af1d74f6a01da1
Reviewed-on: https://go-review.googlesource.com/c/go/+/168977
Reviewed-by: Cherry Zhang <cherryyz@google.com>
It is possible that a "volatile" value (one that can be clobbered
by preparing args of a call) to be used in multiple write barrier
calls. We used to copy the volatile value right before each call.
But this doesn't work if the value is used the second time, after
the first call where it is already clobbered. Copy it before
emitting any call.
Fixes#30977.
Change-Id: Iedcc91ad848d5ded547bf37a8359c125d32e994c
Reviewed-on: https://go-review.googlesource.com/c/go/+/168677
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This CL instrinsifies Add64 with arm64 instruction sequence ADDS, ADCS
and ADC, and optimzes the case of carry chains.The CL also changes the
test code so that the intrinsic implementation can be tested.
Benchmarks:
name old time/op new time/op delta
Add-224 2.500000ns +- 0% 2.090000ns +- 4% -16.40% (p=0.000 n=9+10)
Add32-224 2.500000ns +- 0% 2.500000ns +- 0% ~ (all equal)
Add64-224 2.500000ns +- 0% 1.577778ns +- 2% -36.89% (p=0.000 n=10+9)
Add64multiple-224 6.000000ns +- 0% 2.000000ns +- 0% -66.67% (p=0.000 n=10+10)
Change-Id: I6ee91c9a85c16cc72ade5fd94868c579f16c7615
Reviewed-on: https://go-review.googlesource.com/c/go/+/159017
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
'SUBQ $-0x80, r' is shorter to encode than 'ADDQ $0x80, r',
and functionally equivalent. Use it instead.
Shaves off a few bytes here and there:
file before after Δ %
compile 25935856 25927664 -8192 -0.032%
nm 4251840 4247744 -4096 -0.096%
Change-Id: Ia9e02ea38cbded6a52a613b92e3a914f878d931e
Reviewed-on: https://go-review.googlesource.com/c/go/+/168344
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When initializing a new object, we're often writing
1) to a location that doesn't have a pointer to a heap object
2) a pointer that doesn't point to a heap object
When both those conditions are true, we can avoid the write barrier.
This CL detects case 1 by looking for writes to known-zeroed
locations. The results of runtime.newobject are zeroed, and we
perform a simple tracking of which parts of that object are written so
we can determine what part remains zero at each write.
This CL detects case 2 by looking for addresses of globals (including
the types and itabs which are used in interfaces) and for nil pointers.
Makes cmd/go 0.3% smaller. Some particular cases, like the slice
literal in #29573, can get much smaller.
TODO: we can remove actual zero writes also with this mechanism.
Update #29573
Change-Id: Ie74a3533775ea88da0495ba02458391e5db26cb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/156363
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
