Experimental support for LLVM 4.0.1 (#1592) and 5.0.0. #2303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Praetonus
previously requested changes
Oct 25, 2017
src/libponyc/codegen/genopt.cc
Outdated
| AllocaInst* replace = new AllocaInst(builder.getInt8Ty(), int_size, "", | ||
| begin); | ||
| #else | ||
| AllocaInst* replace = new AllocaInst(builder.getInt8Ty(), | ||
| (unsigned int)int_size->getZExtValue(), "", begin); |
There was a problem hiding this comment.
The new signature of the array AllocaInst constructor is
explicit AllocaInst(Type *Ty, unsigned AddrSpace,
Value *ArraySize = nullptr,
const Twine &Name = "",
Instruction *InsertBefore = nullptr);This means that this is currently using the non-array constructor which isn't taking a size, and is passing the intended size as the address space. This call should instead be
AllocaInst* replace = new AllocaInst(builder.getInt8Ty(), 0, int_size, "", begin);
SeanTAllen
reviewed
Oct 25, 2017
README.md
Outdated
| @@ -250,6 +250,8 @@ Pony requires one of the following versions of LLVM: | |||
| - 3.7.1 | |||
There was a problem hiding this comment.
The unsupported version should be removed
SeanTAllen
reviewed
Oct 25, 2017
README.md
Outdated
| @@ -497,7 +499,7 @@ make default_pic=true | |||
|
|
|||
| You need to have the development versions of the following installed: | |||
|
|
|||
| * LLVM 3.7.1, 3.8.1, or 3.9.1 | |||
| * LLVM 3.7.1, 3.8.1, 3.9.1, 4.0.1, or 5.0.0 | |||
There was a problem hiding this comment.
unsupported versions should be removed
SeanTAllen
reviewed
Oct 25, 2017
README.md
Outdated
| @@ -565,7 +567,7 @@ Please note that on 32-bit X86, using LLVM 3.7.1 or 3.8.1 on FreeBSD currently p | |||
| ## Building on Mac OS X | |||
| [](https://travis-ci.org/ponylang/ponyc) | |||
|
|
|||
| You'll need llvm 3.7.1, 3.8.1 or 3.9.1 and the pcre2 library to build Pony. You can use either homebrew or MacPorts to install your dependencies. Please note that llvm 3.9.1 is not available via homebrew. As such, the instructions below install 3.8.1 | |||
| You'll need llvm 3.7.1, 3.8.1, 3.9.1, 4.0.1, or 5.0.0 and the pcre2 library to build Pony. You can use either homebrew or MacPorts to install your dependencies. Please note that llvm 3.9.1 is not available via homebrew. As such, the instructions below install 3.8.1 | |||
SeanTAllen
reviewed
Oct 25, 2017
README.md
Outdated
| @@ -565,7 +567,7 @@ Please note that on 32-bit X86, using LLVM 3.7.1 or 3.8.1 on FreeBSD currently p | |||
| ## Building on Mac OS X | |||
| [](https://travis-ci.org/ponylang/ponyc) | |||
|
|
|||
| You'll need llvm 3.7.1, 3.8.1 or 3.9.1 and the pcre2 library to build Pony. You can use either homebrew or MacPorts to install your dependencies. Please note that llvm 3.9.1 is not available via homebrew. As such, the instructions below install 3.8.1 | |||
| You'll need llvm 3.7.1, 3.8.1, 3.9.1, 4.0.1, or 5.0.0 and the pcre2 library to build Pony. You can use either homebrew or MacPorts to install your dependencies. Please note that llvm 3.9.1 is not available via homebrew. As such, the instructions below install 3.8.1 | |||
There was a problem hiding this comment.
a later version of llvm is available on OSX via homebrew. this should be updated to use that.
There was a problem hiding this comment.
What is the latest version available via homebrew?
SeanTAllen
reviewed
Oct 25, 2017
| @@ -113,6 +113,66 @@ matrix: | |||
| - ICC1=gcc-6 | |||
There was a problem hiding this comment.
3.7.1 and 3.8.1 tests should be removed.
There was a problem hiding this comment.
We won't be removing these yet because LLVM >= 4 is still in the "experimental" stage.
I added a note to #2371 to remove these when that is resolved.
|
I have removed builds and references to 3.7.1 and 3.8.1. In the README, I'm not sure if replacing '3.8' or '3.9' with '5.0' in the names of various packages is actually valid. |
|
Note that this PR contains a workaround for the LLVM issue, not an actual fix. |
chalcolith
dismissed
Praetonus’s stale review
I have made the requested change.
|
Does anybody with access to MacOS know what happened to the 4.0.1 builds? |
|
@kulibali you want to install |
|
I've done some tests on my Arch Linux system after applying this PR to master and found it can pass 965 tests of the 1064 tests in libponyc.tests when I exclude tests with: It turns out the compiler fails on the very first set of test cases in CodegenOptimisationTest.* In CodegenOptimisationTest there are only two tests and if you run them together it usually fails: But not always: Also it has "allways" works if I run the tests one at a time: If I use We see that the If I look at *thread and *p we see that thread->pool and p are pointing at apparently bad memory: Is there any other information needed or something else I can do to track down the problem? |
|
You also need to build the stdlib tests ( |
winksaville
added a commit
to winksaville/ponyc-rpm-bin
that referenced
this pull request
Nov 19, 2017
This installs ponyc using `makepkg -si` from the release .rpm files on [bintray](https://bintray.com/pony-language/ponyc-rpm). The current ponyc package installed via pacman is not compatible with LLVM5 and fails, see Arch Linux bug [55170](https://bugs.archlinux.org/task/55170) archlinux bug. It is also not possible to build ponyc from source for the same reason. [Issue 2303](ponylang/ponyc#2303) is tracking the problem.
|
given the problems with getting pony installed on rolling distros like Arch, thoughts on merging this and isolating and performance hits to LLVM 4 and 5 and we can have a big shiny warning that there are definitely performance issues with the bug workaround? |
|
I need to update this PR to deal with the changes from #2334. |
|
Ok, the branch is all rebased and updated. Some informal benchmarks, running |
|
@kulibali I've tried to build the packages/stdlib tests but the compilation fails: I did some searching and I'm guessing the problem is that Arch is using libssl.so.1.1 and SSL_library_init and the other undefines is no longer available, see openssl library initialization. The problem might be resvoled by passing OPENSSL_API_COMPAT and OPENSSL_MIN_API with an appropriate value such as 0x10000000L or 0x00908000L. If someone can tell me how I might pass defines when building the standard library I can give it a try. |
|
@winksaville there’s an issue re OpenSSL dependency. This would be best discussed there. |
I'd personally be more comfortable with merging this without the workaround and warn that the support is experimental. The reason is that I don't think that the workaround fixes every case of the bug, given the LLVM change that introduced it. |
|
From sync: I will do the following:
|
This change includes the following: - Compiler code updates for changes to the LLVM 4.0.0 and 5.0.0 API. - Updates to Type-Based Alias Analysis metadata format for LLVM 4.0.0 and up. Note that this code contains a workaround for a problem with hoisting loads of stack-allocated references (see ponylang#2303, ponylang#2061, ponylang#1592), and should be considered experimental. Do not use LLVM 4 or 5 for critical production builds.
chalcolith
added
changelog - added
|
Ok, I have made the changes I note above, and squashed and rebased. This should be ready to go. |
|
My only remaining doubt is that this really increases our build load on Travis CI - I'm wondering if we should consider removing 4.0.1 from the build matrix and depend on our testing of 5.0.0 until the #2371 is resolved and we remove the old version of LLVM from the build matrix. However, I'm not going to hold up this PR on that account - we can consider doing it as a followup. Great work here, everyone! |
jemc
changed the title
ponylang-main
added a commit
that referenced
this pull request
Nov 25, 2017
Closed
lukecheeseman
pushed a commit
to lukecheeseman/ponyc
that referenced
this pull request
Feb 17, 2018
This commit adds support for LLVM >= 4, but disables some stack allocation performance optimizations to do so. As such, we've marked this support as experimental until the optimization can be reinstated.
lukecheeseman
pushed a commit
to lukecheeseman/ponyc
that referenced
this pull request
May 28, 2018
This commit adds support for LLVM >= 4, but disables some stack allocation performance optimizations to do so. As such, we've marked this support as experimental until the optimization can be reinstated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change includes the following: