The name of the function should mention division.
Eliminate double negatives from the comment describing it.
Change-Id: Icef1a5139b3a91b86acb930af97938f5160f7342
Reviewed-on: https://go-review.googlesource.com/c/go/+/217001
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
This requires threading location information through varCount.
This provides much more useful error messages.
Change-Id: If5ff942cbbbf386724eda15a523c181c137fac20
Reviewed-on: https://go-review.googlesource.com/c/go/+/216221
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
We had three implementations.
Refactor, and document the shared implementation.
While we're here, improve the docs for func unbalanced.
Change-Id: I612cce79de15a864247afe377d3739d04a56b9bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/216219
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
CL 213703 converted generated rewrite rules for commutative ops
to use loops instead of duplicated code.
However, it loaded args using expressions like
v.Args[i] and v.Args[i^1], which the compiler could
not eliminate bounds for (including with all outstanding
prove CLs).
Also, given a series of separate rewrite rules for the same op,
we generated bounds checks for every rewrite rule, even though
we were repeatedly loading the same set of args.
This change reduces both sets of bounds checks.
Instead of loading v.Args[i] and v.Args[i^1] for commutative loops,
we now preload v.Args[0] and v.Args[1] into local variables,
and then swap them (as needed) in the commutative loop post statement.
And we now load all top level v.Args into local variables
at the beginning of every rewrite rule function.
The second optimization is the more significant,
but the first helps a little, and they play together
nicely from the perspective of generating the code.
This does increase register pressure, but the reduced bounds
checks more than compensate.
Note that the vast majority of rewrite rules evaluated
are not applied, so the prologue is the most important
part of the rewrite rules.
There is one subtle aspect to the new generated code.
Because the top level v.Args are shared across rewrite rules,
and rule evaluation can swap v_0 and v_1, v_0 and v_1
can end up being swapped from one rule to the next.
That is OK, because any time a rule does not get applied,
they will have been swapped exactly twice.
Passes toolstash-check -all.
name old time/op new time/op delta
Template 213ms ± 2% 211ms ± 2% -0.85% (p=0.000 n=92+96)
Unicode 83.5ms ± 2% 83.2ms ± 2% -0.41% (p=0.004 n=95+90)
GoTypes 737ms ± 2% 733ms ± 2% -0.51% (p=0.000 n=91+94)
Compiler 3.45s ± 2% 3.43s ± 2% -0.44% (p=0.000 n=99+100)
SSA 8.54s ± 1% 8.32s ± 2% -2.56% (p=0.000 n=96+99)
Flate 136ms ± 2% 135ms ± 1% -0.47% (p=0.000 n=96+96)
GoParser 169ms ± 1% 168ms ± 1% -0.33% (p=0.000 n=96+93)
Reflect 456ms ± 3% 455ms ± 3% ~ (p=0.261 n=95+94)
Tar 186ms ± 2% 185ms ± 2% -0.48% (p=0.000 n=94+95)
XML 251ms ± 1% 250ms ± 1% -0.51% (p=0.000 n=91+94)
[Geo mean] 424ms 421ms -0.68%
name old user-time/op new user-time/op delta
Template 275ms ± 1% 274ms ± 2% -0.55% (p=0.000 n=95+98)
Unicode 118ms ± 4% 118ms ± 4% ~ (p=0.642 n=98+90)
GoTypes 983ms ± 1% 980ms ± 1% -0.30% (p=0.000 n=93+93)
Compiler 4.56s ± 6% 4.52s ± 6% -0.72% (p=0.003 n=100+100)
SSA 11.4s ± 1% 11.1s ± 1% -2.50% (p=0.000 n=96+97)
Flate 168ms ± 1% 167ms ± 1% -0.49% (p=0.000 n=92+92)
GoParser 204ms ± 1% 204ms ± 2% -0.27% (p=0.003 n=99+96)
Reflect 599ms ± 2% 598ms ± 2% ~ (p=0.116 n=95+92)
Tar 227ms ± 2% 225ms ± 2% -0.57% (p=0.000 n=95+98)
XML 313ms ± 2% 312ms ± 1% -0.37% (p=0.000 n=89+95)
[Geo mean] 547ms 544ms -0.61%
file before after Δ %
compile 21113112 21109016 -4096 -0.019%
total 131704940 131700844 -4096 -0.003%
Change-Id: Id6c39e0367e597c0c75b8a4b1eb14cc3cbd11956
Reviewed-on: https://go-review.googlesource.com/c/go/+/216218
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This change introduces a new syntax for rewrite rules
that only change a Value's Op. See #36380 for more discussion.
Updating rewrite rules to use ellipses will happen
in follow-up CLs.
Change-Id: I8c56e85de24607579d79729575c89ca80805ba5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/213898
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
It's much easier to debug when you can see
the contents in order to interpret the error message.
Change-Id: I03bbb9dd3071aeca9577cc725a60d43f78118cf4
Reviewed-on: https://go-review.googlesource.com/c/go/+/215717
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
This documentation was lost in CL 213703.
This change restores it.
Change-Id: I544f15771d8a7390893efbda93478b46095ccf3c
Reviewed-on: https://go-review.googlesource.com/c/go/+/215541
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Part of converting all tests to the script framework, but also
just working around flakiness when adding t.Parallel to
TestExecutableGOROOT.
Also, undo the changes to copyFile in golang.org/cl/220317 because
they didn't help.
Fixes#37306
Change-Id: I3348fa1d8f2589febe89604ac19a21df20075c4c
Reviewed-on: https://go-review.googlesource.com/c/go/+/220319
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
An experiment to see if this helps flakiness go away in
TestExecutableGOROOT.
Updates #37306
Change-Id: I2f4f63bdb507359ca07267d86cdb41fe4968f151
Reviewed-on: https://go-review.googlesource.com/c/go/+/220317
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Old trace viewer stopped working with Chrome M80+ because the
old trace viewer heavily depended on WebComponents V0 which are deprecated.
Trace viewer recently migrated to use WebComponents V0 polyfill
(crbug.com/1036492). This CL brings in the newly updated trace_viewer_full.html
(sync'd @ 9508452e)
and updates the javascript snippet included in the /trace endpoint
to use the polyfill.
This brings in webcomponents.min.js copied from
https://chromium.googlesource.com/catapult/+/9508452e18f130c98499cb4c4f1e1efaedee8962/third_party/polymer/components/webcomponentsjs/webcomponents.min.js
That is necessary because the /trace endpoint needs to import
the vulcanized trace_viewer_full.html.
It's possible that some features are not working correctly with
this polyfill. In that case, report the issue to crbug.com/1036492.
There will be a warning message in the UI (yellow banner above the timeline)
which can be hidden by clicking the 'hide' button.
This allows to render the trace in browsers other than chrome in theory,
but I observed some buttons and functions still don't work outside
chrome.
Fixes#34374.
Change-Id: Ib575f756f5e6b22ad904ede6e4d224a995ebe259
Reviewed-on: https://go-review.googlesource.com/c/go/+/219997
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
I noticed some instances of "[ " and " ]" in the rewrite rules.
Normalizing them helps catch possible future duplicate rules.
Change-Id: I892fd7e9b4019ed304f0a61fa2bb7f7e47ef8f38
Reviewed-on: https://go-review.googlesource.com/c/go/+/213682
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Prior to this change, we generated additional rules at rulegen time
for all possible combinations of args to commutative ops.
This is simple and works well, but leads to lots of generated rules.
This in turn has increased the size of the compiler,
made it hard to compile package ssa on small machines,
and provided a disincentive to mark some ops as commutative.
This change reworks how we handle commutative ops.
Instead of generating a rule per argument permutation,
we generate a series of nested loops, one for each commutative op.
Each loop tries both possible argument orderings.
I also considered attempting to canonicalize the inputs to the
rewrite rules. However, because either or both arguments might be
nothing more than an identifier, and because there can be arbitrary
conditions to evaluate during matching, I did not see how to proceed.
The duplicate rule detection now sorts arguments to commutative ops,
so that it can detect commutative-only duplicates.
There may be further optimizations to the new generated code.
In particular, we may not be removing as many bounds checks as before;
I have not investigated deeply. If more work here is needed,
we could do it with more hints or with improvements to the prove pass.
This change has almost no impact on the generated code.
It does not pass toolstash-check, however. In a handful of functions,
for reasons I do not understand, there are minor position changes.
For the entire series ending at this change,
there is negligible compiler performance impact.
The compiler binary shrinks by about 15%,
and package ssa shrinks by about 25%.
Package ssa also compiles ~25% faster with ~25% less memory.
Change-Id: Ia2ee9ceae7be08a17342319d4e31b0bb238a2ee4
Reviewed-on: https://go-review.googlesource.com/c/go/+/213703
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Copied from go1.14.html, with changes redacted back to TODOs following
the model of CL 195058.
'relnote -html' does not report any changes at this time.
Updates #33738
Change-Id: I580232805ab7db35935f3e1ba03b720be4796a7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/220278
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The commutative rule generator has an optimization
where given (Add x y) with no other uses of x and y in the matching rule,
it doesn't generate the commutative match (Add y x).
However, if there is also a condition referring to x or y,
such as (Add x y) && isFoo(x), then we should generate the commutative rule.
This change parses the condition, extracts all idents, and takes them
into consideration.
This doesn't yield any new optimizations now.
However, it is the right thing to do;
otherwise we'll have to track it down and fix it again later.
It is also expensive now, in terms of additional generated code.
However, it will be much, much less expensive soon,
once our generated code for commutative ops gets smaller.
Change-Id: I52c2016c884bbc7789bf8dfe9b9c56061bc028ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/213702
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
HMUL is commutative. However, it has asymmetric register requirements.
There are existing rewrite rules to place arguments in preferable slots.
Due to a bug, the existing rulegen commutativity engine doesn't generate
the commuted form of the HMUL rules.
The commuted form of those rewrite rules cause infinite loops.
In order to fix the rulegen commutativity bug,
we need to choose between eliminating
those rewrite rules and marking HMUL ops as not commutative.
This change chooses the latter, since doing so yields better
optimization results on std+cmd.
Removing the rewrite rules yields only text size regressions:
file before after Δ %
runtime.s 477257 477269 +12 +0.003%
time.s 83552 83612 +60 +0.072%
encoding/asn1.s 57378 57382 +4 +0.007%
cmd/go/internal/modfetch/codehost.s 89822 89829 +7 +0.008%
cmd/internal/test2json.s 9459 9466 +7 +0.074%
cmd/go/internal/test.s 57665 57678 +13 +0.023%
Marking HMUL as not commutative actually yields (mostly) improvements:
file before after Δ %
runtime.s 477257 477247 -10 -0.002%
math.s 35985 35992 +7 +0.019%
strconv.s 53486 53462 -24 -0.045%
syscall.s 82483 82446 -37 -0.045%
time.s 83552 83561 +9 +0.011%
os.s 52691 52684 -7 -0.013%
archive/zip.s 42285 42272 -13 -0.031%
encoding/asn1.s 57378 57329 -49 -0.085%
encoding/base64.s 12156 12094 -62 -0.510%
net.s 296286 296276 -10 -0.003%
encoding/base32.s 9720 9658 -62 -0.638%
net/http.s 560931 560907 -24 -0.004%
net/smtp.s 14421 14411 -10 -0.069%
cmd/vendor/golang.org/x/sys/unix.s 74307 74266 -41 -0.055%
The regressions are minor, and are in functions math.cbrt,
time.Time.String, and time.Date.
Change-Id: I9f6d9ee71654e5b70381cac77b0ac26011f4ea12
Reviewed-on: https://go-review.googlesource.com/c/go/+/213701
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
When working on rulegen, I often find myself
searching the rules files to find the source of
generated code. Add a flag to make that easier.
The flag needs to be off by default,
so that adding a single rule doesn't cause a massive diff.
Change-Id: I5a6f09129dc6fceef7c9cd1ad7eee24f3880ba91
Reviewed-on: https://go-review.googlesource.com/c/go/+/213700
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
If the two commutative arguments are perfectly identical,
then swapping them will never have an effect.
Passes toolstash-check for the relevant architectures,
that is, linux-386, linux-386-387, linux-amd64, linux-s390x.
Change-Id: I19f91644867d8d174bd01f872abe4809013872ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/213698
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I6db652a4a515daf6e87645d34191dc9a441f5720
Reviewed-on: https://go-review.googlesource.com/c/go/+/214431
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I1cb3e2e28700b05b08933f4e24cd996268c1f163
Reviewed-on: https://go-review.googlesource.com/c/go/+/214428
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I2d14c07c590cc618c66f27fdc3a2bb8120c6d646
Reviewed-on: https://go-review.googlesource.com/c/go/+/214427
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I16fb0910196c96caef6ed380f96010a548407f9e
Reviewed-on: https://go-review.googlesource.com/c/go/+/214424
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I2170f427c238e9fe8c029b43b346621d82c5e8fc
Reviewed-on: https://go-review.googlesource.com/c/go/+/214388
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I69c69809fb1698c8198ef3ea00103a9acb7b6ce7
Reviewed-on: https://go-review.googlesource.com/c/go/+/214387
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: Ib1c55a48fafb5ce040ac70707bbc2a3ee5e2ddd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/214382
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
golang.org/cl/214141 introduced the typo 'skup' for 'skip', which
broke tests. This change fixes it.
Change-Id: I1b3c230b545f1c093d3e0feedc3b41f3f0b41bec
Reviewed-on: https://go-review.googlesource.com/c/go/+/220157
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I27e52c4eabfcd1782965f17c098719dd0ea7e3ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/214390
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I1328a87e2481b4555b01df5c898f1a8015412adc
Reviewed-on: https://go-review.googlesource.com/c/go/+/214296
Reviewed-by: Jay Conrod <jayconrod@google.com>
Adds a in-script go binary called inarchive to check that an
archive is produced. A weaker could be done faster using grep,
but this is more faithful to the original test.
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I001fa0698063be80fe3da947c81d4eb0829be47f
Reviewed-on: https://go-review.googlesource.com/c/go/+/214295
Reviewed-by: Jay Conrod <jayconrod@google.com>
This one's a bit complex and required writing support go programs
within the test script.
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I9e91225b20b1b043f032b77a55c5825cb9d9a4b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/214292
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: Ie46118eddbd7c3ed0bb9ecee4bdc1cb6fdaf06a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/214291
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I0696fa54184514d69c0763ac772d99b12e133eb0
Reviewed-on: https://go-review.googlesource.com/c/go/+/214288
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: Ie8b54b272e8a04720e437a37a5e5b0afd73481b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/214285
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I3465cad1b0ba0d912067429146f1cb0668d5aa6b
Reviewed-on: https://go-review.googlesource.com/c/go/+/214284
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I1f49a84f91735f39d5922c1347e79298780149c7
Reviewed-on: https://go-review.googlesource.com/c/go/+/214218
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I1de2b428ea7dac5429020742bf12bea910a02079
Reviewed-on: https://go-review.googlesource.com/c/go/+/214141
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: Icc14d4188574badf3da71d34857616f2a2ad5862
Reviewed-on: https://go-review.googlesource.com/c/go/+/214138
Reviewed-by: Jay Conrod <jayconrod@google.com>
Part of converting all tests to script framework to improve
test parallelism.
Updates #36320
Updates #17751
Change-Id: I72bbba0a20a8731a89e1b4f4c9ac13b21c080cd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/214119
Reviewed-by: Jay Conrod <jayconrod@google.com>
