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

[Emil Velikov]

On 12 October 2016 at 00:02, Tobias Droste <tdroste at gmx.de> wrote:
> Consolidate the required LLVM versions at the top where the other
> versions for dependencies are listed.
> Rework the LLVM_VERSION and LLVM_VERSION_INT calculation to make the
> format "x.y.z." possible.
>
Not sure how others feel, but I love this.

Disclaimer: there's a lot of "follow-up" suggestion below. They are
just ideas that come to mind, not something that you are expected to
do :-)

> LLVM_VERSION_INT is now including LLVM_VERSION_PATCH.
> For the C define HAVE_LLVM is set now which does not include
> LLVM_VERSION_PATCH.
>
> Signed-off-by: Tobias Droste <tdroste at gmx.de>
> ---
>  configure.ac | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 4847704..0d1530c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -93,6 +93,15 @@ XVMC_REQUIRED=1.0.6
>  PYTHON_MAKO_REQUIRED=0.8.0
>  LIBSENSORS_REQUIRED=4.0.0
>
> +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.

> +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.

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 ?

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" .

> -        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 ?

> -        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.


>  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'`

-Emil