[Mesa-dev] [PATCH 1/6] configure.ac: Rename MESA_LLVM to FOUND_LLVM

[Tobias Droste]

Am Donnerstag, 8. Dezember 2016, 16:59:27 CET schrieb Emil Velikov:
> On 8 December 2016 at 02:03, Tobias Droste <tdroste at gmx.de> wrote:
> > this renames MESA_LLVM to FOUND_LLVM and updates the config.log report
> > to say if LLVM is found or not, to make clear that this does not mean
> > that it is used.
> > 
> > Signed-off-by: Tobias Droste <tdroste at gmx.de>
> > ---
> > 
> >  configure.ac | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index adca49d31e..1499380c45 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -997,15 +997,15 @@ llvm_set_environment_variables() {
> > 
> >          fi
> >          
> >          DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT
> >          -DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH"> 
> > -        MESA_LLVM=1
> > +        FOUND_LLVM=yes
> > 
> >      else
> > 
> > -        MESA_LLVM=0
> > +        FOUND_LLVM=no
> > 
> >          LLVM_VERSION_INT=0
> >      
> >      fi
> >  
> >  }
> >  
> >  llvm_check_version_for() {
> > 
> > -    if test "x$MESA_LLVM" = x0; then
> > +    if test "x$FOUND_LLVM" = xno; then
> > 
> >          AC_MSG_ERROR([LLVM $1 or newer is required for $2])
> >          return
> >      
> >      fi
> > 
> > @@ -1065,7 +1065,6 @@ radeon_llvm_check() {
> > 
> >  llvm_set_environment_variables
> > 
> > -AC_SUBST([MESA_LLVM])
> > 
> >  AC_SUBST([LLVM_BINDIR])
> >  AC_SUBST([LLVM_CFLAGS])
> >  AC_SUBST([LLVM_CPPFLAGS])
> > 
> > @@ -2507,7 +2506,7 @@ if test -n "$with_gallium_drivers"; then
> > 
> >              ;;
> >          
> >          xswrast)
> >          
> >              HAVE_GALLIUM_SOFTPIPE=yes
> > 
> > -            if test "x$MESA_LLVM" = x1 && test "x$enable_gallium_llvm" ==
> > "xyes";  then +            if test "x$FOUND_LLVM" = xyes && test
> > "x$enable_gallium_llvm" == "xyes";  then
> Something I've noticed a bit too late - using == may work, but is incorrect.
> Sorry about that :-\

Oh yes, you are right! Will post a v2 as soon as we figured out what to do 
with the other patches.

> 
> Maybe
> if test "x$FOUND_LLVM" = xyes -a "x$enable_gallium_llvm" = "xyes";  then
> 
> >                  HAVE_GALLIUM_LLVMPIPE=yes
> >              
> >              fi
> >              ;;
> > 
> > @@ -2566,7 +2565,7 @@ dnl by calling llvm-config --libs
> > ${DRIVER_LLVM_COMPONENTS}, but> 
> >  dnl this was causing the same libraries to be appear multiple times
> >  dnl in LLVM_LIBS.
> > 
> > -if test "x$MESA_LLVM" != x0; then
> > +if test "x$FOUND_LLVM" != xno; then
> > 
> >      if ! $LLVM_CONFIG --libs ${LLVM_COMPONENTS} >/dev/null; then
> >      
> >         AC_MSG_ERROR([Calling ${LLVM_CONFIG} failed])
> > 
> > @@ -2666,7 +2665,7 @@ AM_CONDITIONAL(NEED_RADEON_DRM_WINSYS, test
> > "x$HAVE_GALLIUM_R300" = xyes -o \> 
> >  AM_CONDITIONAL(NEED_WINSYS_XLIB, test "x$enable_glx" = xgallium-xlib)
> >  AM_CONDITIONAL(NEED_RADEON_LLVM, test x$NEED_RADEON_LLVM = xyes)
> >  AM_CONDITIONAL(HAVE_GALLIUM_COMPUTE, test x$enable_opencl = xyes)
> > 
> > -AM_CONDITIONAL(HAVE_GALLIUM_LLVM, test "x$MESA_LLVM" = x1 -a \
> > +AM_CONDITIONAL(HAVE_GALLIUM_LLVM, test "x$FOUND_LLVM" = xyes -a \
> > 
> >                                         "x$enable_gallium_llvm" = xyes)
> >  
> >  AM_CONDITIONAL(USE_VC4_SIMULATOR, test x$USE_VC4_SIMULATOR = xyes)
> >  if test "x$USE_VC4_SIMULATOR" = xyes -a "x$HAVE_GALLIUM_ILO" = xyes; then
> > 
> > @@ -2948,12 +2947,12 @@ else
> > 
> >  fi
> >  
> >  echo ""
> > 
> > -if test "x$MESA_LLVM" = x1; then
> > -    echo "        llvm:            yes"
> > +if test "x$FOUND_LLVM" = xyes; then
> > +    echo "        llvm found:      yes"
> > 
> >      echo "        llvm-config:     $LLVM_CONFIG"
> >      echo "        llvm-version:    $LLVM_VERSION"
> >  
> >  else
> > 
> > -    echo "        llvm:            no"
> > +    echo "        llvm found:      no"
> > 
> >  fi
> 
> In hindsight - even if we say "found" here, it might be confusing. If
> so, the alternative is to either a) track all the places which require
> LLVM in the above/below guards ro b) have a require_llvm which is set
> by each consumer.

The tracking is done by patch 2. Every consumer checks for a specific llvm 
version. That's more or less a fool proof way of tracking.
I could split it out into a separate function but it would be called after/
before each version check so there's not really something to gain.

After patch 4 you get something like this:

        llvm found:      yes
        llvm-config:     <somepath>/llvm-config
        llvm-version:    4.0.0
        llvm used:       yes

        Gallium drivers: swrast
        Gallium st:      mesa
        Gallium llvm:    no


This means llvm was found, is used in one or more locations, but is disabled 
for gallium. 

The result for gallium llvm would be:

        llvm found:      yes
        llvm-config:     <somepath>/llvm-config
        llvm-version:    4.0.0
        llvm used:       yes

        Gallium drivers: swrast
        Gallium st:      mesa
        Gallium llvm:    yes

The result without llvm at all:

        llvm found:      no
        llvm used:       no

        Gallium drivers: swrast
        Gallium st:      mesa
        Gallium llvm:    no

Tobias

> 
> Personally I'm fine either way - "found" or extra tracking to print
> LLVM info only as needed.
> With the == nitpick the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> -Emil