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

Upgrade to LLVM 3.9 #19123

Closed
StefanKarpinski opened this issue Oct 26, 2016 · 34 comments · Fixed by #19678
Closed

Upgrade to LLVM 3.9 #19123

StefanKarpinski opened this issue Oct 26, 2016 · 34 comments · Fixed by #19678
Milestone

Comments

@StefanKarpinski
Copy link
Member

Related but closed: #18863. What's the status of this?

@vchuravy
Copy link
Member

vchuravy commented Oct 27, 2016

#19054 had the important bits and pieces of #18863 and from what I can tell we need:

@vchuravy vchuravy added this to the 0.6.0 milestone Oct 27, 2016
@tkelman
Copy link
Contributor

tkelman commented Oct 27, 2016

@tkelman
Copy link
Contributor

tkelman commented Oct 27, 2016

Note that the threads patch was initially borrowed from rust (who have the same toolchain C++11 threads issue we do with mingw), so we should check if they have a rebased version we could use to save ourselves a bit of time there. (edit: they do)

@vchuravy
Copy link
Member

vchuravy commented Oct 28, 2016

Something else I just noticed is that the Intel Compiler 2016_update2 is not able to build llvm. See https://sft.its.cern.ch/jira/browse/ROOT-8233 and https://software.intel.com/en-us/forums/intel-c-compiler/topic/640702

@tkelman
Copy link
Contributor

tkelman commented Oct 28, 2016

Looks like that's icc's bug, but llvm continues to do an incredible job of finding new ways to break every other compiler...

@ihnorton
Copy link
Member

ihnorton commented Oct 28, 2016

Note that the threads patch was initially borrowed from rust (who have the same toolchain C++11 threads issue we do with mingw)

Worth keeping an eye on: https://github.com/lhmouse/mcfgthread

There was some discussion on the gcc list in April, but I don't see any response or recent activity, so it is unclear if/when it might go in to mainline. There is some further discussion/development on mingw-w64-public.

lhmouse does have a forked toolchain build here.

(related: https://github.com/meganz/mingw-std-threads -- header-only, but does not implement call_once std::async, and a handful of other things. call_once was the only issue I encountered when I tried building LLVM against it a while back)

@tkelman
Copy link
Contributor

tkelman commented Oct 28, 2016

Yes, though unless that somehow gets backported to gcc 4, we'll need to worry about https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77333... or come up with a proper fix to gmp's build system so it works with clang on windows and start using that (ref #15488 which doesn't work in cross-compile).

@Keno
Copy link
Member

Keno commented Oct 28, 2016

Another issue we'll start seeing is that LLVM symbols conflict with graphics drivers on Linux distributions that have (stupidly) chosen to dynamically link LLVM into their graphics drivers. I have a patch (https://reviews.llvm.org/D13330) that adds capabilities to clang, to avoid generating the problematic symbols, but I doubt similar capabilities will make it into gcc anytime soon.

@kpamnany
Copy link
Contributor

kpamnany commented Nov 2, 2016

Does this work at all on Linux? Need LLVM 3.9 to start testing Julia performance on KNL.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 2, 2016

Stock/patched LLVM 3.9.0 should work with no problem on x86/x64 linux.

@andreasnoack
Copy link
Member

@kpamnany I had to use LLVM-svn on KNL but it worked fine (~a month ago.)

@yuyichao
Copy link
Contributor

yuyichao commented Nov 2, 2016

@kpamnany Apply https://reviews.llvm.org/D26123 or a commit before https://reviews.llvm.org/rL285426 if you want a llvm-svn build without assertion enabled.

@StefanKarpinski
Copy link
Member Author

Bump. What's the status here?

@tkelman
Copy link
Contributor

tkelman commented Nov 12, 2016

#19138 can be merged, but bootstrap crashes on both win32 and win64 last time I tried with 3.9.

@StefanKarpinski
Copy link
Member Author

What's necessary to get this not to crash?

@ViralBShah
Copy link
Member

@Keno is necessary, perhaps?

maleadt added a commit that referenced this issue Nov 14, 2016
Use of the NVPTX back-end is only tested on LLVM 3.9 (and hence depends on #19123),
in combination with the CUDAnative.jl package.
maleadt added a commit that referenced this issue Nov 15, 2016
Use of the NVPTX back-end is only tested on LLVM 3.9 (and hence depends on #19123),
in combination with the CUDAnative.jl package.
maleadt added a commit that referenced this issue Nov 15, 2016
Use of the NVPTX back-end is only tested on LLVM 3.9 (and hence depends on #19123),
in combination with the CUDAnative.jl package.
@vchuravy
Copy link
Member

vchuravy commented Dec 5, 2016

With #19138 both Win32 and Win64 build successfully and with #19493 my local Win64 builds passes all tests.

I didn't have much time to look into the Win32 build, but I am seeing a failure in the threading tests for a = Atomic{Int64}(); a[] = 1.

@tkelman
Copy link
Contributor

tkelman commented Dec 15, 2016

@vchuravy that's what https://reviews.llvm.org/D27653 is intended to address, right?

@yuyichao
Copy link
Contributor

yuyichao commented Dec 15, 2016

Yes. But I don't think that's the right solution. I think the long term solution is to make sure we can find libatomic on windows (which can possibly be MSVC compatible once MSVC supports it and if gcc devs do the right thing) and the simplest short term solution is to remove szclass 12 on 32bit.

@tkelman
Copy link
Contributor

tkelman commented Dec 15, 2016

remove szclass 12 on 32bit

What effect would that have? Things are working without requiring libatomic on llvn 3.7. There actually is no toolchain option that can build llvm 3.7 (and the rest of release-0.5 julia) correctly on win32 and includes libatomic as far as I'm aware.

@yuyichao
Copy link
Contributor

What effect would that have?

Larger memory consumption.

Things are working without requiring libatomic on llvn 3.7

Due to a LLVM bug.

@tkelman
Copy link
Contributor

tkelman commented Dec 15, 2016

What other consequences does the llvm bug have? A memory regression on 32 bit might be fine if it's not too large. Can we avoid libatomic or do we need to fix gcc 5+ (or libgmp's build system) to be able to have a toolchain that can build both llvm 3.9 and release-0.5 julia?

@yuyichao
Copy link
Contributor

What other consequences does the llvm bug have?

The instructions it generates violates the platform ABI, which shouldn't cause too much issue due to our limited use of it currently.

Can we avoid libatomic

That's why I mentioned a short term solution? Or are you talking about something else?

@tkelman
Copy link
Contributor

tkelman commented Dec 15, 2016

crossing threads in my head here, sorry. what needs patching to remove szclass 12?

@yuyichao
Copy link
Contributor

yuyichao commented Dec 15, 2016

1857f07 apart from the MAX_ALIGN part. It's quite easy to do if we decide that we need a short term solution (i.e. if we cannot get libatomic to work before we want to use llvm39 on windows).

@tkelman
Copy link
Contributor

tkelman commented Dec 16, 2016

So modifying the ifdef conditionals there to also include win32? What about i686 linux? Not sure whether llvm 3.9 has been tested there yet but we have buildbots that run tests there.

I can test this if anyone can propose the right patch; so could anyone else who has a win32 source build handy but I'm guessing there aren't that many aside from me.

@yuyichao
Copy link
Contributor

So modifying the ifdef conditionals there to also include win32

Yes.

What about i686 linux

I suspect this is less useful than win32 so may as well include it as well if we are going to change it on win32.

Not sure whether llvm 3.9 has been tested there yet

I've tested it and the tests should all pass.

I can test this if anyone can propose the right patch

If we decide to do this I'll write the patch. Or maybe we can test it first anyway?

@vchuravy
Copy link
Member

From my point of view removing szclass 12 on 32bit (both Linux and Windows) is the right thing to do for now and once we can upgrade GCC we can revisit this.

I can test this over the weekend.

@vchuravy
Copy link
Member

#19647 implements @yuyichao suggestion and I tested it on 32bit linux and windows (via wine)

@tkelman
Copy link
Contributor

tkelman commented Dec 20, 2016

Tests pass for me with 3.9.0. We're in that annoying period where LLVM has tagged 3.9.1 but not released source tarballs, so maybe we should make our own again temporarily (if someone does this, may as well try to be consistent with how llvm makes their source releases look, in terms of a .src or whatever naming convention they use inside the tarball).

Someone will need to submit an llvm39-julia homebrew formula to the homebrew-julia tap (should leave the llvm37-julia alone as it is, best not to delete it), may as well make it 3.9.1 plus all our patches for consistency with how source builds will look.

maleadt added a commit that referenced this issue Dec 22, 2016
Use of the NVPTX back-end is only tested on LLVM 3.9 (and hence depends on #19123),
in combination with the CUDAnative.jl package.
@ViralBShah
Copy link
Member

Noting the homebrew tap for llvm39 PR here: staticfloat/homebrew-julia#229

@vchuravy It seems that all the items in your list above are carried out. Can you checkmark them all if appropriate?

@ViralBShah
Copy link
Member

Should we close this one and focus on #19678 ?

@tkelman
Copy link
Contributor

tkelman commented Dec 27, 2016

#19678 will close this when merged.

@ViralBShah
Copy link
Member

ViralBShah commented Dec 27, 2016

Ah I missed that #19678 was a PR. Sorry about the noise.

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 a pull request may close this issue.

9 participants