-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Bump LLVM version to 6.0.0 #26398
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
Bump LLVM version to 6.0.0 #26398
Conversation
|
Looks like about 5% slower, which is quite decent, and perhaps even recoverable quickly - should we decide to do something like this. |
|
👍 I think we should try to upgrade LLVM before 1.0, so that the 0.7-1.0 timeframe is the least stable period, and we can steadily increase reliability and performance over the 1.0.x and 1.x series. |
|
@vchuravy Just wondering if you have a comparison of the total time for running tests. |
deps/llvm.mk
Outdated
| $(eval $(call LLVM_PATCH,llvm-PPC-addrspaces)) # PPC | ||
| $(eval $(call LLVM_PATCH,llvm-PR36292-5.0)) # PPC fixes #26249, remove for 6.0 | ||
| else ifeq ($(LLVM_VER_SHORT),6.0) | ||
| #$(eval $(call LLVM_PATCH,llvm-5.0.0_threads)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just fyi the comment on this one "Cygwin and openSUSE still use win32-threads mingw" hasn't been true for over a year, ref https://build.opensuse.org/request/show/444383 and https://sourceware.org/ml/cygwin/2016-11/msg00113.html. should be safe to drop this patch nowadays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! That is good news.
|
@maleadt Is LLVM 6.0 good from the GPU perspective? Just pinging you here in case you haven't seen it. |
|
Yeah, I was waiting for the AS patch before testing this (if Valentin doesn't beat me to it, I'll do it next week). |
|
already ported just not pushed yet, vchuravy@2c19817 |
|
On Nanosoldier4 (with -j8): 6.0.03.9.1Builds |
|
@nanosoldier |
|
It seems nanosoldier lately only listens to its true master @ararslan |
|
@nanosoldier |
|
He still ignores me too, like a toddler acting out. @nanosoldier |
|
Could be confused by the branch target changing, as it seems both Travis and AppVeyor are. I'd restart the server but there's another build running. |
|
@nanosoldier |
|
CircleCI is encountering a Codegen issue. Win32 is failing during building LLVM Wind64 is having a link time issue: |
|
Please add https://reviews.llvm.org/D42260 to the patch list. |
|
@nanosoldier |
|
Sure, You haven't landed it yet, right? |
|
No, I should though. |
|
Quick update on the codegen failure. It is not triggered by the debug build, but rather by |
|
Could you include D44140? With it, LLVM.jl and CUDAnative.jl test without issues. |
|
@nanosoldier |
|
Man, Nanosoldier really hates this PR. @nanosoldier |
|
@nanosoldier |
|
Good news. After updating a copy of the buildbots from mingw 5.4 to 6.2 and including the last three patches I got Julia to build and pass tests on Win64! |
|
@vchuravy What do you need in order to check off the |
|
armv7l finished the builds, but I don't think the buildbot managed to run
the tests due to the qemu syscall issue?
ppc64le is more complicated. Right now we are triggering an assertion and I
am trying to reduce the issue so that I can bisect it and report it
upstream.
…On Sat, 17 Mar 2018 at 19:10 Elliot Saba ***@***.***> wrote:
@vchuravy <https://github.com/vchuravy> What do you need in order to
check off the armv7l and ppc64le boxes on your checklist?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI3agorDIPad3l0eT3Ng138Cbvudanfks5tfZf0gaJpZM4Sk0LP>
.
|
|
+1 from me. It compiled fine on Arch Linux, and tests for the LLVM.jl package ran fine. I also tried recompiling with the WebAssembly backend included, and that worked fine, too. (Once this is in, I'll probably submit a PR to include the wasm backend.) |
|
Thanks Tom! When you submit a PR for the WebAssembly backend just make sure
to measure increase in compile time for LLVM and increase in binary size,
but as long as it builds on all platforms it shouldn't be a problem to
include it.
…On Sun, Mar 18, 2018, 12:48 Tom Short ***@***.***> wrote:
+1 from me. It compiled fine on Arch Linux, and tests for the LLVM.jl
package ran fine. I also tried recompiling with the WebAssembly backend
included, and that worked fine, too. (Once this is in, I'll probably submit
a PR to include the wasm backend.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI3ar4zhFq7yJdNI252MGTpyLnBwvYXks5tfo_SgaJpZM4Sk0LP>
.
|
|
Once I replace 4436b90 with https://reviews.llvm.org/D44650 this should be good to go, but I will give upstream a couple of days to respond to that. There is also movement on the |
|
Should delete the old checksums. And Mac and Windows CI are not actually testing LLVM 6 yet: https://travis-ci.org/JuliaLang/julia/jobs/354081449#L6185, https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.25526/job/pdt7a2i4xvn8nvj7#L2596 |
|
I tested Mac and Windows via manually triggering the buildbots. But we will
need to make sure to update the cache files for windows and presumably
homebrew bottles for Mac.
…On Mon, 19 Mar 2018 at 19:41 Tony Kelman ***@***.***> wrote:
Should delete the old checksums. And Mac and Windows CI are not actually
testing LLVM 6 yet:
https://travis-ci.org/JuliaLang/julia/jobs/354081449#L6185,
https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.25526/job/pdt7a2i4xvn8nvj7#L2596
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI3anBOi_4du1DV54-zar8t4FwlJLdsks5tgEIegaJpZM4Sk0LP>
.
|
|
Some (encouraging?) benchmarks. Defining a fixed size array, defining methods for unrolled matmat and matvec multiplication as in as in https://gist.github.com/KristofferC/07814a1dc4e6dfc07697ca2452b309da, and disabling/enabling the SLP-vectorizer and running the following benchmark: n = 4
v4 = FixedVector((rand(n)...,))
m4x4 = FixedMatrix{n, n, Float64, n*n}((rand(n*n)...,))
n = 8
v8_32 = FixedVector((rand(Float32, n)...,))
m8x8_32 = FixedMatrix{n, n, Float32, n*n}((rand(Float32, n*n)...,))
using BenchmarkTools
@btime $m4x4 * $m4x4;
@btime $m4x4 * $v4;
@btime $m8x8_32 * $m8x8_32;
@btime $m8x8_32 * $v8_32;I get:
LLVM 6 with SLP enabled does a very good job. 0.7 with current LLVM seems to fail to vectorize in all cases even with SLP. |
|
Hmm, on my machine with 0.6 -O3: julia> using StaticArrays, BenchmarkTools
julia> @btime a * b setup = (a = rand(SMatrix{4, 4}); b = rand(SMatrix{4, 4}))
9.261 ns (0 allocations: 0 bytes)and with your gist: @btime $m4x4 * $m4x4;
20.611 ns StaticArrays doesn't do any explicit SIMD stuff. Why are these baseline results so different? |
|
I used the definitions in https://github.com/JuliaCI/BaseBenchmarks.jl/blob/master/src/tuple/TupleBenchmarks.jl. These were originally taken from StaticArrays but perhaps they have been updated? |
|
With an added julia> @btime $m4x4 * $m4x4;
9.242 ns (0 allocations: 0 bytes) |
- forward-ports the NVPTX address space changes - drops the Windows specific threading changes - add patch for D42260 - add patch for D44140 - backport patch for FastISel bug - final version of ssp patch for Windows - fix CMAKE issue for mingw32 compilers
|
Ok, does anybody have any other comments on this. I would like to merge this PR this week and from my side it is ready (modulo the infrastructure work). |
|
Don't think LLVM_VER should be changed for source builds until CI is actually testing it. |
|
That's what I meant by "modulo the infrastructure work":
My plan is the following:
1. Switch builds servers to Mingw32 6.2
2. Regenerate the llvm-juliadeps archive (is there documentation for this?)
3. Update homebrew bottle
Am I missing anything?
…On Mon, Mar 26, 2018, 10:56 Tony Kelman ***@***.***> wrote:
Don't think LLVM_VER should be changed for source builds until CI is
actually testing it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI3atsPeIB6Og2Ix27oPupndpsnl2dXks5tiSvGgaJpZM4Sk0LP>
.
|
Why? That's not what CI will use, or anyone following the README.windows.md instructions. |
|
On the current version of mingw that the buildbots are using I get a linker failure that makes little sense to me (#26398 (comment)) on 6.2 that failure goes away. I managed to fix a similar linker failure in https://reviews.llvm.org/D44650, but that was prevalent on both 5.4 and 6.2. |
|
Closed in favour of #26925 |
Viral and Jeff cornered me and asked me how hard it would be to switch to 6.0.0.
This is intended as a smoke-screen test to see what CI and more importantly Nanosoldier says.
FastISel#264166.0.0
time -v:3.9.1
time -v:Edit: GPU testing to the list above by @ViralBShah