[Mesa-dev] [PATCH v3 25/25] configure.ac: Add required LLVM versions to the top

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 11:53:50 CEST schrieb Emil Velikov:
> > 
> > +dnl LLVM versions
> > +LLVM_VERSION_REQUIRED_GALLIUM=3.3.0
> > +LLVM_VERSION_REQUIRED_LLVMPIPE=3.6.0
> 
> Not exactly sure why/how llvmpipe got 3.6 since the driver reuses the
> "gallium" one.

Yes you're right. I'm going to change this to 3.3.0.

> 
> > +LLVM_VERSION_REQUIRED_OPENCL=3.6.0
> > +LLVM_VERSION_REQUIRED_R600=3.6.0
> > +LLVM_VERSION_REQUIRED_RADEONSI=3.6.0
> 
> There's a small related gotcha: as-is at build time we get the
> different codepaths thus, as people build against shared LLVM (hello
> Archlinux, I'm looking at you) and update their LLVM without
> rebuilding mesa (Arch I'm looking at you again) things go funny.
> 
> Tl;Dr; We really want to enable static linking by default and prod
> distros to use it.

I'm all in favor of statically linking LLVM (that's the way I'm doing this on 
my pc). 
I think the only reason this is not done is because people (also here on the 
list) don't want any static linkg of external libraries because of size or 
whatever.
So changing the default to static is easy, but I doubt it will make everyone 
happy ;-)

> 
> As an alternative (or in parallel really) we could bump to 3.6.2...
> 
> > +LLVM_VERSION_REQUIRED_RADV=3.9.0
> > +LLVM_VERSION_REQUIRED_SWR=3.6.0
> 
> ... and 3.6.1 + drop the older codepaths. Just a follow-up idea.
> 
> > +
> 
> Nit: Drop _VERSION part ?

Ok.

> 
> Follow-up: worth checking with VMWare guys (Jose et al.) it they're
> still using pre-3.6 llvm and doing code/configure cleanups.
> 
> >  dnl Check for progs
> >  AC_PROG_CPP
> >  AC_PROG_CC
> > 
> > @@ -935,29 +944,38 @@ llvm_get_version() {
> > 
> >              [#include "${LLVM_INCLUDEDIR}/llvm/Config/llvm-config.h"])
> >          
> >          AC_COMPUTE_INT([LLVM_VERSION_MINOR], [LLVM_VERSION_MINOR],
> >          
> >              [#include "${LLVM_INCLUDEDIR}/llvm/Config/llvm-config.h"])
> > 
> > +        AC_COMPUTE_INT([LLVM_VERSION_PATCH], [LLVM_VERSION_PATCH],
> > +            [#include "${LLVM_INCLUDEDIR}/llvm/Config/llvm-config.h"])
> > +
> > +        if test -z "$LLVM_VERSION_PATCH"; then
> > +            LLVM_VERSION_PATCH=`echo $LLVM_VERSION | cut -d. -f3 | egrep
> > -o '^[[0-9]]+'` +        fi
> 
> IIRC the LLVM_VERSION parsing was added by the AMD folks since old
> versions did not have the define. Might be worth checking with them
> (and llvm headers/codebase) if that's no longer the case given their
> min. required version.
> 
> Just an idea, in parallel to the above "require !=0 minor for swr/radeonsi"

3.6.0 has this already, so if only amd needs this, I'm going to drop this.

> .
> > -        LLVM_VERSION_PATCH=`echo $LLVM_VERSION | cut -d. -f3 | egrep -o
> > '^[[0-9]]+'`> 
> >          if test -z "$LLVM_VERSION_PATCH"; then
> >          
> >              LLVM_VERSION_PATCH=0
> >          
> >          fi
> >          
> >          if test -n "${LLVM_VERSION_MAJOR}"; then
> > 
> > -           
> > LLVM_VERSION_INT="${LLVM_VERSION_MAJOR}0${LLVM_VERSION_MINOR}"
> > -        else
> > -            LLVM_VERSION_INT=`echo $LLVM_VERSION | sed -e
> > 's/\([[0-9]]\)\.\([[0-9]]\)/\10\2/g'` +           
> > LLVM_VERSION="${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_
> > PATCH}"> 
> >          fi
> 
> Follow-up idea: just bail out if the LLVM_VERSION_* defines are not
> set, and reuse those to construct HAVE_LLVM amongst others (dropping
> the sed 'magic') or use them all together in favour of our local
> macros ?

The sed magic is needed there to add the leading 0 to the minor version if 
it's < 10:

3.6.1		-> 3061
3.10.2 		-> 3102

If I can always use the header provided values this could be done easily with 
some if < 9 then ... else ... fi

> 
> > -        DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT
> > -DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH" +       
> > LLVM_VERSION_INT=`echo $LLVM_VERSION | sed -e
> > 's/\.\([[1-9]][[0-9]]\)\.\([[0-9]]\)/\1\2/' -e
> > 's/\.\([[0-9]]\)\.\([[0-9]]\)/0\1\2/' -e 's/\.\([[1-9]][[0-9]]\)/\10/' -e
> > 's/\.\([[0-9]]\)/0\10/'` +        HAVE_LLVM=`echo $LLVM_VERSION | sed -e
> > 's/\.\([[1-9]][[0-9]]\)\.\([[0-9]]\)/\1/' -e
> > 's/\.\([[0-9]]\)\.\([[0-9]]\)/0\1/' -e 's/\.\([[1-9]][[0-9]]\)/\1/' -e
> > 's/\.\([[0-9]]\)/0\1/'` +
> 
> The sed patterns have changed, please elaborate [a bit] why/how in the
> commit msg.

This just makes the conversion for double digit minor numbers work. It didn't 
work with the old script.

> 
> >  llvm_check_version_for() {
> > 
> > -    if test "${LLVM_VERSION_INT}${LLVM_VERSION_PATCH}" -lt
> > "${1}0${2}${3}"; then -        AC_MSG_ERROR([LLVM $1.$2.$3 or newer is
> > required for $4]) +    llvm_target_version=`echo $1 | sed -e
> > 's/\.\([[1-9]][[0-9]]\)\.\([[0-9]]\)/\1\2/' -e
> > 's/\.\([[0-9]]\)\.\([[0-9]]\)/0\1\2/' -e 's/\.\([[1-9]][[0-9]]\)/\10/' -e
> > 's/\.\([[0-9]]\)/0\10/'` +
> 
> A something as simple as the following should do it as well, shouldn't it ?
>   llvm_target_version=`echo $1 | sed 's/\.//g'`

Sadly no, because of the leading zero in case there are double digit minor 
numbers.

> 
> -Emil