RFC: Use JL_LLVM_VERSION instead of LLVMxx #17826

ararslan · 2016-08-05T02:08:58Z

As noted in the linked issue, the names LLVM37, LLVM34, etc. aren't immediately descriptive because they don't refer to specific versions but rather to ranges of versions. This implements the proposed fix of having a single LLVM_VERSION macro that holds the LLVM version as a two-digit number and instead of checking whether LLVMxx is defined, we check whether LLVM_VERSION >= xx. Similarly, checking whether LLVMxx is not defined equates to LLVM_VERSION < xx.

This works locally, so fingers crossed that it works everywhere else...

yuyichao · 2016-08-05T02:13:59Z

There was some discussion of whether there should be a 3.10 version. Just to be safe, I think we can use something like 0x030701.

ararslan · 2016-08-05T02:15:00Z

src/llvm-version.h

+ #elif LLVM_VERSION_MINOR == 3
+ #define LLVM_VERSION 33
+ #endif
+ #endif


Constructing LLVM_VERSION can be done more succinctly as

#define CONCAT(a, b) a ## b #define GETVERS(maj, min) CONCAT(maj, min) #define LLVM_VERSION GETVERS(LLVM_VERSION_MAJOR, LLVM_VERSION_MINOR)

But as @yuyichao noted, this is the kind of thing that one gets stabbed for if put into production.

ararslan · 2016-08-05T02:16:55Z

You mean check for LLVM 3.10 by doing LLVM_VERSION >= 0x030701?

yuyichao · 2016-08-05T02:17:57Z

No, I mean #define JL_LLVM_VERSION (LLVM_VERSION_MAJOR * 0x10000 + LLVM_VERSION_MINOR * 0x100 + LLVM_VERSION_PATCH)

yuyichao · 2016-08-05T02:27:34Z

100 instead of 0x100 is also fine, and can possibly reduce typo (it's probably more likely to miss a 0x than adding one)

yuyichao · 2016-08-05T02:29:19Z

Anyther way is to use something like the glibc trick

julia/src/threading.c

Line 153 in 3e00217

# if __GLIBC_PREREQ(2, 12)

ararslan · 2016-08-05T02:33:21Z

So something like

#if _LLVM_VERSION_GE(3, 10)

where _LLVM_VERSION_GE(x, y) checks that the version is at least x.y?

ararslan · 2016-08-05T02:36:07Z

I get segfaults when I do #define JL_LLVM_VERSION (LLVM_VERSION_MAJOR * 0x10000 + LLVM_VERSION_MINOR * 0x100 + LLVM_VERSION_PATCH)

yuyichao · 2016-08-05T02:36:54Z

Yes.... Maybe use JL_ instead of _ as prefix. IIRC, there is one place we check patch version in this file. Too bad msvc doesn't like variadic macros otherwise we can use the same macro to check version with an optional patch version....

yuyichao · 2016-08-05T02:39:21Z

I get segfaults when I do

Full patch??

ararslan · 2016-08-05T02:58:37Z

What do you mean full patch? This is what I was doing to test:

#define LLVM_VERSION_MAJOR 3
#define LLVM_VERSION_MINOR 10
#define LLVM_VERSION_PATCH 1

#define JL_LLVM_VERSION (LLVM_VERSION_MAJOR * 0x10000 + LLVM_VERSION_MINOR * 0x100 + LLVM_VERSION_PATCH)

int main() {
    int x = JL_LLVM_VERSION;
    printf("%s", x);
    return 0;
}

yuyichao · 2016-08-05T03:01:30Z

    int x = JL_LLVM_VERSION;
    printf("%s", x);
    return 0;

Why should this work at all?

ararslan · 2016-08-05T03:06:00Z

¯_(ツ)_/¯ Because I'm not good at C

yuyichao · 2016-08-05T03:10:02Z

Because I'm not good at C

A good advice for C is to turn on -Wall -Wextra and fix all the warning.

ararslan · 2016-08-05T03:24:51Z

Oh, heh, whoops. Format string...

So for major version 3, minor 10, patch 1, I get 199169. How would you check the version based on that number?

yuyichao · 2016-08-05T03:28:59Z

0x31001

ararslan · 2016-08-05T03:29:39Z

Do patches ever have more than two digits?

yuyichao · 2016-08-05T03:33:37Z

Do patches ever have more than two digits?

The nope is that neither patch and minor do. Actually 100 is better than 0x100 sin 3.10 is actually 0x30a00 ........

ararslan · 2016-08-05T05:00:22Z

Okay, changed to use the 0x version you suggested.

yuyichao · 2016-08-05T05:01:51Z

I actually mean the 100 version is better ....

yuyichao · 2016-08-05T05:03:15Z

src/llvm-version.h


-#endif
+ #if LLVM_VERSION < 0x33000


I think thse bounds are wrong. E.g. after switching to 100, this should be 30300

yuyichao · 2016-08-05T05:16:47Z

LGTM other than the missing JL_ prefix.

ararslan · 2016-08-05T05:18:55Z

Okay, added the JL_ prefix. I had meant to add it when you had first mentioned it but forgot. Thanks so much for all of your help and patience!

yuyichao · 2016-08-05T05:48:15Z

src/codegen_internal.h

@@ -1,6 +1,6 @@
 // This file is a part of Julia. License is MIT: http:https://julialang.org/license

-#if defined(LLVM38) && !defined(LLVM37)
+#if defined(JL_LLVM_VERSION) && JL_LLVM_VERSION >= 30800


Is the defined(JL_LLVM_VERSION) needed here?

No, that's not needed. I'll take it out.

IMHO, we shouldn't check if JL_LLVM_VERSION is defined. This can catch errors of missing include earlier rather than silently ignoring the error.

yuyichao · 2016-08-05T05:53:09Z

I looked through the diff and I think the translation is correct. Since the diff is so long, it would be better if someone else could have a look too to make sure we aren't silently disabling features on newer version of llvm.

vtjnash · 2016-08-05T06:06:45Z

src/jitlayers.cpp

@@ -103,7 +103,7 @@ void addOptimizationPasses(PassManager *PM)
 #endif

 #if defined(JL_ASAN_ENABLED)
-# if defined(LLVM37) && !defined(LLVM38)
+# if JL_LLVM_VERSION == 30700


#if JL_LLVM_VERSION >= 30700 && JL_LLVM_VERSION < 30800

tkelman · 2016-08-06T02:34:28Z

src/codegen_internal.h

@@ -1,6 +1,6 @@
 // This file is a part of Julia. License is MIT: http:https://julialang.org/license

-#if defined(LLVM38) && !defined(LLVM37)
+#if JL_LLVM_VERSION >= 30800


how would the old version of this ever happen?

The function is also declared in jitlayers.cpp. Doing it here might have some issue due to missing type declaration/missing headers.

@tkelman If 3.8 is the lowest version. So it's at least 3.8 and not 3.7 or lower.

@yuyichao Should the function declaration here be removed then?

The condition is actually USE_ORCJIT && JL_LLVM_VERSION <= 30800 (roughly 3.7 <= llvm_ver < 3.8). It is better to remove the declaration in jitlayers.cpp instead and keep this one with the condition fixed.

The condition there before was false, the current condition is wrong. The correct one is in jitlayers.cpp and is the same with the one I gave above.

Looks like it fails if I remove the part in jitlayers.cpp.

Move to jitlayers.h

defined(LLVM38) && !defined(LLVM37) would never happen since LLVM37 meant "3.7 or newer"

Which is why I said,

The condition there before was false

This was added before the JIT and codegen are split and therefore before jitlayers.h exists. This is needed only by jitlayers.cpp, which doesn't include codegen_internal.h, for LLVM 3.7.x which is why it should be moved to jitlayers.h.

tkelman · 2016-09-08T06:18:01Z

src/codegen_internal.h

@@ -1,6 +1,6 @@
 // This file is a part of Julia. License is MIT: http:https://julialang.org/license

-#if defined(LLVM38) && !defined(LLVM37)
+#if defined(USE_ORCJIT) && JL_LLVM_VERSION <= 30800


since the old version of this could never be true, probably best to just delete these few lines

I think @yuyichao specifically requested this change: #17826 (comment)

tkelman · 2016-09-08T06:27:03Z

(I found this a little easier to review locally after

curl -L https://github.com/JuliaLang/julia/pull/17826.patch | git apply
for i in $(seq 3 9); do
  sed -i -e "s/JL_LLVM_VERSION >= 30${i}00/defined(LLVM3$i)/g" \
    -e "s/JL_LLVM_VERSION < 30${i}00/\!defined(LLVM3$i)/g" \
    -e "s/#if defined(LLVM3$i)/#ifdef LLVM3$i/g" \
    -e "s/#if \!defined(LLVM3$i)/#ifndef LLVM3$i/g" src/*.cpp src/*.h
done

which gives a shorter diff)

ararslan · 2016-09-15T02:00:42Z

Is there anything else this needs before it's good to go?

yuyichao · 2016-09-15T20:24:52Z

src/debuginfo.cpp

@@ -59,7 +59,7 @@ using namespace llvm;
 #include <cstdio>
 #include <cassert>

-#if defined(LLVM35) && !defined(LLVM36)
+#if JL_LLVM_VERSION == 30500


This should in principle still be a range since LLVM 3.5.1 does exist. Not sure if it actually matters though since we might not compile there anyway....

Fixed, thanks!

yuyichao · 2016-09-15T20:28:55Z

LGTM other than the very minor comment above. I think we should merge this now. It's unlikely that we can trivially backport all codegen changes anymore and the conflict introduced by this change should be easy to resolve.

If everyone is fine with this I'd like to merge this tomorrow (with the comment above fixed or not) and rebase #18519 on top of this.

yuyichao · 2016-09-16T04:28:15Z

I think there was a conflict before #18519 was merged........ So I guess this needs to be rebased again (hopefully the last time).

ararslan · 2016-09-16T04:56:52Z

Rebased and conflicts resolved. Thanks for the heads up!

ararslan · 2016-09-16T17:23:04Z

Reviewers should probably pay extra careful attention to future PRs that modify or introduce LLVM version conditioning in the C preprocessor, since #ifdef LLVMxx will always be false now.

tkelman · 2016-09-16T17:30:58Z

should we make those defines result in compile-time errors now? maybe just for a little while?

ararslan · 2016-09-16T17:33:08Z

That seems like a good idea to me. How would you propose we go about that? I can submit a PR.

yuyichao · 2016-09-16T17:56:32Z

I don't think there's a way to do it in C++ code (a grep on CI could do that though). It shouldn't be a huge issue since hopefully PR's that uses those are already tested on newer LLVM versions.

ararslan reviewed Aug 5, 2016
View reviewed changes

yuyichao reviewed Aug 5, 2016
View reviewed changes

vtjnash reviewed Aug 5, 2016
View reviewed changes

tkelman reviewed Aug 6, 2016
View reviewed changes

ararslan force-pushed the aa/llvm_version_macro branch from a6d63b6 to bca6f94 Compare August 7, 2016 01:15

ararslan force-pushed the aa/llvm_version_macro branch 5 times, most recently from cef63d2 to 0158e6c Compare September 8, 2016 00:51

tkelman reviewed Sep 8, 2016
View reviewed changes

ararslan force-pushed the aa/llvm_version_macro branch 3 times, most recently from dd37fe8 to 8cc0383 Compare September 14, 2016 22:32

ararslan changed the title ~~RFC: Use LLVM_VERSION instead of LLVMxx~~ RFC: Use JL_LLVM_VERSION instead of LLVMxx Sep 14, 2016

ararslan force-pushed the aa/llvm_version_macro branch from 8cc0383 to b83e1bd Compare September 14, 2016 22:34

yuyichao reviewed Sep 15, 2016

View reviewed changes

ararslan force-pushed the aa/llvm_version_macro branch from b83e1bd to d9e07b5 Compare September 15, 2016 20:34

Use JL_LLVM_VERSION instead of LLVMxx in the preprocessor

038abd2

ararslan force-pushed the aa/llvm_version_macro branch from d9e07b5 to 038abd2 Compare September 16, 2016 04:53

yuyichao merged commit 8cb72ee into JuliaLang:master Sep 16, 2016

ararslan deleted the aa/llvm_version_macro branch September 16, 2016 17:21

RFC: Use JL_LLVM_VERSION instead of LLVMxx #17826

RFC: Use JL_LLVM_VERSION instead of LLVMxx #17826

Conversation

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

Choose a reason for hiding this comment

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

yuyichao commented Aug 5, 2016

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016 • edited Loading

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

yuyichao commented Aug 5, 2016

Choose a reason for hiding this comment

yuyichao commented Aug 5, 2016

ararslan commented Aug 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented Aug 5, 2016

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

Choose a reason for hiding this comment

tkelman commented Sep 8, 2016

ararslan commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented Sep 15, 2016 • edited Loading

yuyichao commented Sep 16, 2016

ararslan commented Sep 16, 2016

ararslan commented Sep 16, 2016

tkelman commented Sep 16, 2016

ararslan commented Sep 16, 2016

yuyichao commented Sep 16, 2016

ararslan commented Aug 5, 2016 •

edited

Loading

yuyichao commented Sep 15, 2016 •

edited

Loading