Update the Pony compiler to be able to use LLVM 4.0.0 (#1592). #2061

chalcolith · 2017-07-19T23:14:39Z

This change includes the following:

Compiler code updates for changes to the LLVM 4.0.0 API.
Updates to Type-Based Alias Analysis metadata format for LLVM 4.0.0.
Builds in Release mode on Windows to conform to the Unix makefile.
Some efficiency tweaks for the Windows build.

SeanTAllen · 2017-07-19T23:18:52Z

.travis.yml

+          packages:
+            - g++-6
+      env:
+        - FAVORITE_CONFIG=yes


can only have 1 favorite config. can you set this to "no" for now.

favorite config is used to decide which linux version will be done for release. so until we decide that 4.0.0 is the default, it should be "no"

SeanTAllen · 2017-07-19T23:20:07Z

getting some build errors:

src/libponyc/codegen/gentype.c:160:21: error: ‘make_tbaa_descptr’ defined but not used [-Werror=unused-function]
 static LLVMValueRef make_tbaa_descptr(LLVMContextRef ctx, LLVMValueRef root)
                     ^~~~~~~~~~~~~~~~~
src/libponyc/codegen/gentype.c:149:21: error: ‘make_tbaa_descriptor’ defined but not used [-Werror=unused-function]
 static LLVMValueRef make_tbaa_descriptor(LLVMContextRef ctx,
                     ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [build/release/obj/libponyc/codegen/gentype.o] Error 1

Praetonus

Awesome, thank you!

This should also contain README changes for LLVM 4. I've also left a few inline comments.

Praetonus · 2017-07-19T23:35:46Z

src/libponyc/codegen/genreference.c


  if(ast_id(def) != TK_STRUCT)
    index++;
-
+  


Whitespace here.

Praetonus · 2017-07-19T23:38:51Z

Makefile

 else
  $(warning WARNING: Unsupported LLVM version: $(llvm_version))
-  $(warning Please use LLVM 3.7.1, 3.8.1, or 3.9.1)
+  $(warning Please use LLVM 3.7.1, 3.8.1, 3.9.1, or 4.0.0)


This should contain 4.0.1 as well.

This wont work with homebrew on the mac until 4.0.1 is in here.

Based on how we do this, we shouldn't be supporting 4.0.0 we should only support 4.0.1

Ok, I wasn't aware of the homebrew status.

IIRC, we're not supporting 3.7.0, 3.8.0 and 3.9.0 because we've found blocking LLVM bugs in these versions. Until we find such a bug in either 4.0.0 or 4.0.1, I think it would be good to officially support both.

i thought we were only "supporting" the most recent version of any of the major releases. if we are supporting 4.0.0 then we need to do CI on it. Homebrew is going to install 4.0.1 on OSX.

The CI is a good point. You're right, we should keep the current scheme.

Praetonus · 2017-07-19T23:45:41Z

src/libponyc/codegen/codegen.h

@@ -179,8 +181,10 @@ typedef struct compile_t
  LLVMDIBuilderRef di;
  LLVMMetadataRef di_unit;
  LLVMValueRef tbaa_root;
+#if LLVM_PONY < 400


This should be PONY_LLVM.

Praetonus · 2017-07-19T23:46:53Z

src/libponyc/codegen/gentype.c

 static LLVMValueRef make_tbaa_root(LLVMContextRef ctx)
 {
  const char str[] = "Pony TBAA";
  LLVMValueRef mdstr = LLVMMDStringInContext(ctx, str, sizeof(str) - 1);
  return LLVMMDNodeInContext(ctx, &mdstr, 1);
 }

-static LLVMValueRef make_tbaa_descriptor(LLVMContextRef ctx, LLVMValueRef root)


Why was this removed? It is still needed for TBAA on type descriptors.

Hmm, you're right. Seems to have been lost when I reverted my over-ambitious TBAA stuff.

Praetonus · 2017-07-19T23:47:02Z

src/libponyc/codegen/gentype.c

-  return LLVMMDNodeInContext(ctx, params, 3);
-}
-
-static LLVMValueRef make_tbaa_descptr(LLVMContextRef ctx, LLVMValueRef root)


Same as above.

Praetonus · 2017-07-19T23:49:54Z

src/libponyc/codegen/host.cc

+{
+  pony_assert(parent != NULL);
+  pony_assert(node != NULL);
+  pony_assert((int)i >= 0);


This check isn't needed. llvm::MDNode::replaceOperandWith has an unsigned parameter so i will never be used as signed.

SeanTAllen · 2017-07-20T00:38:51Z

@kulibali you need to update .travis_install.bash to correctly install LLVM 4.0 on Travis.

You'd need OSX and Linux versions.

SeanTAllen · 2017-07-20T00:48:47Z

.travis_install.bash

+    brew install pcre2
+    brew install libressl
+
+    brew install [email protected]


this is incorrect. there is no [email protected]

i would be:

brew install llvm

SeanTAllen · 2017-07-20T00:49:08Z

.travis_install.bash

+    brew install libressl
+
+    brew install [email protected]
+    brew link --overwrite --force [email protected]


you don't want [email protected] here

SeanTAllen · 2017-07-20T00:51:46Z

.travis_install.bash

+    brew link --overwrite --force [email protected]
+    mkdir llvmsym
+    ln -s "$(which llvm-config)" llvmsym/llvm-config-4.0
+    ln -s "$(which clang++)" llvmsym/clang++-4.0


im not sure that these are needed. i'd say turn these 3 lines and the corresponding export PATH off and we can enable if needed.

Would this be OK for the OSX section?

"osx:llvm-config-4.0") brew update brew install shellcheck shellcheck ./.*.bash ./*.bash brew install pcre2 brew install libressl brew install llvm # brew link --overwrite --force llvm # mkdir llvmsym # ln -s "$(which llvm-config)" llvmsym/llvm-config-4.0 # ln -s "$(which clang++)" llvmsym/clang++-4.0 # do this elsewhere: #export PATH=llvmsym/:$PATH ;;

SeanTAllen · 2017-07-20T00:52:33Z

Makefile

-    LLVM_LINK = /usr/local/opt/[email protected]/bin/llvm-link
-    LLVM_OPT = /usr/local/opt/[email protected]/bin/opt
+	ifneq (,$(shell which /usr/local/opt/[email protected]/bin/llvm-config 2> /dev/null))
+    LLVM_CONFIG = /usr/local/opt/[email protected]/bin/llvm-config


there's no [email protected]

it installs into /usr/local/opt/[email protected]/bin/

Do you mean /usr/local/opt/llvm/bin?

chalcolith · 2017-07-20T03:44:19Z

Ok, I'll need to build new Windows libraries to support 4.0.1, and then I'll update the Windows build. I don't have any access to OSX so I'll try to get it right but will probably need some help.

Praetonus · 2017-07-21T12:28:30Z

src/libponyc/codegen/gentype.c

 static LLVMValueRef make_tbaa_root(LLVMContextRef ctx)
 {
  const char str[] = "Pony TBAA";
  LLVMValueRef mdstr = LLVMMDStringInContext(ctx, str, sizeof(str) - 1);
  return LLVMMDNodeInContext(ctx, &mdstr, 1);
 }

+#if PONY_LLVM < 400
+
 static LLVMValueRef make_tbaa_descriptor(LLVMContextRef ctx, LLVMValueRef root)


Unfortunately this still isn't correct. The two functions are needed even for LLVM 4. The created metadata nodes are used when loading an element of a type descriptor. See for example desc_field in gendesc.c.

For LLVM 4 they need to be in the new path-aware format. I had done some work on this but not finished it. I'll take a further look.

chalcolith · 2017-07-21T16:28:24Z

I'm not sure what the Linux test errors are about. On my Ubuntu 16.04 machine all the tests pass.

I'm going to need some help with the OSX travis setup, as I have no access to an OSX machine.

Praetonus · 2017-07-21T18:27:23Z

@kulibali I'll take a look, to see if it segfaults on my machine.

Praetonus · 2017-07-21T21:00:31Z

I can reproduce the segfault. At first I suspected that it came from the descriptor TBAA stuff but I fixed that and it still happens. I'll continue to investigate.

Here's the diff for the TBAA.

Praetonus · 2017-07-21T21:22:06Z

I've narrowed the crash down to the HeapToStack optimisation pass. It looks like a change in LLVM 4 made previously valid transformations invalid.

chalcolith · 2017-07-21T21:36:40Z

Thanks! I have applied your patch. The tests still pass on Ubuntu 16.04 for me.

Praetonus · 2017-07-29T18:41:10Z

Status update on the crash: it looks like the problem is related to the LICM pass in LLVM, which is hoisting loads and sinking stores in invalid places, resulting in reads from uninitialised memory. I'm still unsure whether the problem is coming from the custom passes in the Pony compiler or the LLVM passes.

I've reduced the crash in collections/persistent to a minimal case. I'll add it here in case somebody else wants to investigate. I suspect it could be reduced even more but I haven't been able to do so yet.

class val MapNode
  let entries: USize = 0

class val HashMap
  let root: MapNode = MapNode

actor Main
  new create(env: Env) =>
    var m6 = HashMap
    let vs = Array[U8].create(2).>push(0).>push(1)
    let i = vs.values()
    while i.has_next() do
      try
        i.next()
        if m6.root.entries != 0 then
          error
        end
        m6 = HashMap
      end
    end

chalcolith · 2017-08-02T21:29:56Z

I have found by trial and error that if I change genopt.cc:276 to case Instruction::Load: return false; I do not encounter the crash. I am still trying to wrap my head around this pass's code, so I don't immediately understand why a load of the stack-allocated pointer would crash.

Praetonus · 2017-08-12T17:29:24Z

@kulibali Unfortunately I don't think that's the problem. That function determines whether a given heap allocation can be stack allocated by looking at the uses of the allocation, on the basis that no use should capture the allocated pointer (e.g. by storing it somewhere).

A load never captures, so I think you're seeing that result because the change to the load case is overriding the hypothetical buggy case which is erroneously saying that the corresponding operation doesn't capture.

chalcolith · 2017-08-19T22:41:24Z

The segfault also disappears when I disable the inliner pass.

SeanTAllen · 2017-08-20T00:22:28Z

Interesting

This change includes the following: - Compiler code updates for changes to the LLVM 4.0.0 API. - Updates to Type-Based Alias Analysis metadata format for LLVM 4.0.0. - Builds in Release mode on Windows to conform to the Unix makefile. - Some efficiency tweaks for the Windows build.

chalcolith · 2017-09-08T04:38:29Z

Just for fun I have made a branch that uses LLVM 5.0.0: https://github.com/kulibali/ponyc/tree/llvm500

It compiles on Ubuntu, but crashes in LLVMLifetimeStart.

chalcolith · 2017-09-08T22:10:47Z

I have fixed the crash in LLVMLifetimeStart. Everything works on Windows, but generated code on Linux still segfaults. If I disable the HeapToStack pass, it works.

Praetonus · 2017-09-09T18:01:12Z

This sounds like the same bug that's blocking the LLVM 4 port.

chalcolith · 2017-10-25T17:48:51Z

This PR is superceded by #2303

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 Automatically add "Added" CHANGELOG entry on merge do not merge This PR should not be merged at this time labels Jul 19, 2017

SeanTAllen reviewed Jul 19, 2017

View reviewed changes

Praetonus suggested changes Jul 19, 2017

View reviewed changes

Praetonus mentioned this pull request Jul 20, 2017

Removing LLVM 3.7 support #2063

Closed

SeanTAllen mentioned this pull request Jul 20, 2017

Last Week in Pony - July 23, 2017 ponylang/ponylang-website#112

Closed

SeanTAllen reviewed Jul 20, 2017

View reviewed changes

Praetonus suggested changes Jul 21, 2017

View reviewed changes

chalcolith force-pushed the llvm400 branch from 41954a5 to 66bd415 Compare August 18, 2017 18:30

chalcolith force-pushed the llvm400 branch from 66bd415 to 5589f45 Compare August 30, 2017 22:54

chalcolith added 4 commits September 1, 2017 13:00

build fixes

68137e6

travis fixes

dc571c2

fixes from @Praetonus comments

b8d0bd6

chalcolith and others added 15 commits September 1, 2017 13:00

appveyor fix

015e5ab

install llvm 4 for travis

a325587

update README to refer to LLVM 4.0.1

7428fbc

update build files to use LLVM 4.0.1

9ae3617

tweak for windows debug builds

bffbfae

correctly comment out old-style tbaa for llvm 4

98de9b6

fix llvm version in appveyor.yml

7a26472

fix llvm version in travis.yml

7013c10

another travis fix

2cc75d7

llvm tbaa fixes from @Praetonus

9c38410

fix warning in makefile

0d3e017

Attempt to get LLVM4 installed for OSX on travis

0618264

Take 2 on getting llvm 4 working on travis osx

ca19668

Travis-install cleanup

40aecaa

rebase merge fixes

19e7f93

chalcolith force-pushed the llvm400 branch from 5589f45 to 19e7f93 Compare September 1, 2017 20:00

chalcolith mentioned this pull request Sep 12, 2017

Add support for LLVM 4 #1592

Closed

This was referenced Oct 13, 2017

additional llvm version compile switches #2282

Closed

Compiler segfault with optimisation enabled (possibly due to 'Combine redundant instructions') #2280

Closed

chalcolith closed this Oct 25, 2017

chalcolith mentioned this pull request Nov 24, 2017

Fix building with LLVM 5 and 6 so that the stack allocation workaround is no longer needed. #2371

Closed

chalcolith deleted the llvm400 branch January 26, 2018 03:36

Update the Pony compiler to be able to use LLVM 4.0.0 (#1592). #2061

Update the Pony compiler to be able to use LLVM 4.0.0 (#1592). #2061

Conversation

chalcolith commented Jul 19, 2017

Choose a reason for hiding this comment

SeanTAllen commented Jul 19, 2017

Praetonus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

SeanTAllen Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalcolith commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalcolith commented Jul 21, 2017

Praetonus commented Jul 21, 2017

Praetonus commented Jul 21, 2017

Praetonus commented Jul 21, 2017

chalcolith commented Jul 21, 2017

Praetonus commented Jul 29, 2017

chalcolith commented Aug 2, 2017

Praetonus commented Aug 12, 2017

chalcolith commented Aug 19, 2017 • edited Loading

SeanTAllen commented Aug 20, 2017

chalcolith commented Sep 8, 2017

chalcolith commented Sep 8, 2017

Praetonus commented Sep 9, 2017

chalcolith commented Oct 25, 2017

SeanTAllen commented Jul 20, 2017 •

edited

Loading

SeanTAllen Jul 20, 2017 •

edited

Loading

chalcolith commented Aug 19, 2017 •

edited

Loading