Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Change LLVM version to 3.9.1 #19678

Merged
merged 3 commits into from
Dec 29, 2016
Merged

RFC: Change LLVM version to 3.9.1 #19678

merged 3 commits into from
Dec 29, 2016

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Dec 22, 2016

update checksums and CI

closes #19123

still needs the homebrew formula to be updated, and could use some compile-time benchmarking to see how much slower it's going to make things

and the x86 / amd64 linux buildbots will need JuliaCI/julia-buildbot#54 done

(some travis failures due to swapping cmake version around backed up to https://gist.github.com/anonymous/df52ea47a8b03cbcf82113456afca167 and https://gist.github.com/anonymous/28e2387302285cdeb5a30602ea07a79d)

@ViralBShah
Copy link
Member

@staticfloat Pinging to see if you can help update the homebrew formula for llvm 3.9.

@ViralBShah
Copy link
Member

I think we can move to 3.9 on master for some time and everyone can try it out over the next few weeks. We can always switch back if necessary.

@tkelman
Copy link
Contributor Author

tkelman commented Dec 22, 2016

@nanosoldier runbenchmarks(ALL, vs = ":master") (this will probably only tell us run time performance differences rather than compile time)

This now builds, and should pass tests, everywhere but mac travis. So this is "request for homebrew" - take a look at https://github.com/staticfloat/homebrew-julia/blob/master/llvm37-julia.rb and https://github.com/Homebrew/homebrew-core/blob/master/Formula/llvm.rb, and add the entire 3.9.1 patch list from deps/llvm.mk (use a recent master sha instead of the 0.5.0-rc2 tag for the patch urls), submit an llvm39-julia.rb as a PR to the staging branch at homebrew-julia (helps if someone with a mac can test the homebrew formula build locally)

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: ErrorException("failed process: Process(`make`, ProcessExited(2)) [2]")

Logs and partial data can be found here
cc @jrevels

@tkelman
Copy link
Contributor Author

tkelman commented Dec 22, 2016

does nanosoldier need to run contrib/download_cmake.sh?

@vchuravy
Copy link
Member

vchuravy commented Dec 22, 2016

Since it is building Julia every time it will need an updated cmake as well @jrevels

@staticfloat
Copy link
Sponsor Member

I'm working on a llvm39-julia formula here.

@jrevels
Copy link
Member

jrevels commented Dec 23, 2016

I've updated Nanosoldier's cmake to 3.7.1. Let's try again:

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@tkelman tkelman changed the title WIP: Change LLVM version to 3.9.1 RFC: Change LLVM version to 3.9.1 Dec 24, 2016
@tkelman
Copy link
Contributor Author

tkelman commented Dec 24, 2016

Benchmarks are all over the place, some improvements and some slowdowns.

Now bottled on homebrew for mac travis (old log at https://gist.github.com/fe6f37594cd18eaab73867768d2bfc88, restarted) so should pass everywhere. How much compile-time benchmarking do we want to do here before merging? Worth a pre-merge pkgeval run? Hopefully most packages shouldn't notice but you never know.

@tkelman tkelman force-pushed the tk/llvm39 branch 2 times, most recently from ecf2462 to 79c4cec Compare December 24, 2016 02:36
update checksums and CI

rebuilt appveyor binaries to include nvptx backend and patches

install llvm39-julia explicitly on mac travis
did this get built without an executable suffix?
can still build against the old copies but we won't validate
those downloads anymore
@vchuravy
Copy link
Member

I think a PkgEval would be interesting, otherwise LGTM. We can then start working on fixing the runtime regressions.
The main metrics for compile time regression would be building of the sysimg and running the test suite?

@ViralBShah
Copy link
Member

Just noting that the homebrew tap is merged and linking the PR here: staticfloat/homebrew-julia#229

@ViralBShah
Copy link
Member

It looks like the slowdowns are all scalar indexing related, which would perhaps also explain the slowdown across the board in all the sorting benchmarks.

@ViralBShah
Copy link
Member

ViralBShah commented Dec 27, 2016

Perhaps we can go ahead and merge this, and tackle the regressions as part of the release process post feature freeze? The performance regressions appear not to be too bad, although it is not clear what the compile time regressions will be like.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Dec 27, 2016

We can estimate compilation time by looking at the time it took for the compilation heavy tests to pass on Travis:

Test LLVM 3.9.1 (s) Master (s) Ratio
linalg/qr 127.97 107.64 1.19
linalg/dense 130.14 111.73 1.16
linalg/triangular 413.94 350.12 1.18
linalg/matmul 165.75 142.48 1.16
subarray 430.66 345.26 1.24
sparse/sparsevector 175.07 151.95 1.15

@ViralBShah
Copy link
Member

Seems like there is an average of 20% slowdown - perhaps mostly attributable to compile times.

@KristofferC
Copy link
Sponsor Member

I think we can confidently say that the slowdowns are only from compile times.

@vchuravy
Copy link
Member

vchuravy commented Dec 28, 2016

I spend the morning investigating the worst runtime regression. ["misc","bitshift",("UInt32","UInt32")] with 3.51.
First of all the regression brings us back to performance level for v0.5 in this particular benchmark so I feel that shouldn't stop us from upgrading.

From my investigations there seem to be two problems:

  1. A change in how register pressure is calculated pessimises interleaving for the loop. I have a half baked fix, that I will submit to LLVM upstream for discussions.
  2. LLVM 3.7 performs a second optimisation that I don't yet understand fully, where it vectorises a step that LLVM 3.9 lifts out of the body. This leads to 2x performance difference, even if 1; is fixed.

Another test I did was to time the sys image build
3.9

real	2m13.072s
user	2m12.597s
sys	0m1.050s

vs 3.7

real	2m3.609s
user	2m2.600s
sys	0m1.003s

@ViralBShah
Copy link
Member

I think this is all good enough to merge. Should we merge this first before the other stuff that is coming from @JeffBezanson ?

@ViralBShah
Copy link
Member

ViralBShah commented Dec 29, 2016

I ran the microbenchmarks, and they are all similar across llvm 3.7.1 and 3.9.1, except for rand_mat_stat, which went from execution time of 19 on 3.7.1 to 23 on 3.9.1.

@tkelman
Copy link
Contributor Author

tkelman commented Dec 29, 2016

another useful test - time for initial using Gadfly without any existing .ji files present
llvm 3.7: 108.3 s
llvm 3.9: 113.7 s

@tkelman tkelman merged commit 19f81ac into master Dec 29, 2016
@tkelman tkelman deleted the tk/llvm39 branch December 29, 2016 18:27
@ViralBShah
Copy link
Member

We will need @Keno to pay attention to all the small regressions here after feature freeze.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to LLVM 3.9
7 participants