Re: [Mesa-dev] [PATCH v3 08/25] configure.ac: Move LLVM version check to the top

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 20:02:24 CEST schrieb Emil Velikov:
> >> With the above
> >> Reviewed-by: Emil Velikov 
> >> 
> >> > +if test -z "${LLVM_CONFIG}"; then
> >> > +if test -n "$llvm_prefix"; then
> >> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no],
> >> > ["$llvm_prefix/bin"]) +else
> >> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
> >> > +fi
> >> > +fi
> >> > +
> >> > +if test "x$LLVM_CONFIG" != xno; then
> >> > +LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
> >> 
> >> ...
> >> 
> >> > +else
> >> > +MESA_LLVM=0
> >> > +LLVM_VERSION_INT=0
> >> 
> >> Just realised that we should error out in this case. After all one
> >> requests llvm, so silently ignoring that they're missing llvm-config
> >> isn't a smart idea. Something like below (be that as a preparatory,
> >> in-between or at the end of the series) would be great.
> > 
> > At this point in time we don't know if we actually need LLVM.
> 
> Looking at the code in master (and at this point in your series) I see
> no way how this can happen.
> Can you point out where/how we can get that ?
> 
> Either way... this is in the "follow-up" ideas category.

After my last patches in the series LLVM is checked without any condition at 
the beginning of the configure process. This way I have this info everywhere.

Later a driver who needs just calls llvm_check_version_for() and this function 
then bails out if there's not LLVM_CONFIG because version 0 is always smaller 
then the requested version.

The user gets a message stating he needs LLVM version x.y.z. for driver d.
Which is more usefull than a general "LLVM is missing" message.

After he gets this message he can either install LLVM or remove the driver 
from --with-gallium-drivers/--with-vulkan-driver

> 
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 08/25] configure.ac: Move LLVM version check to the top

[Emil Velikov]

On 12 October 2016 at 19:11, Tobias Droste  wrote:
> Am Mittwoch, 12. Oktober 2016, 10:11:50 CEST schrieb Emil Velikov:
>> On 12 October 2016 at 00:02, Tobias Droste  wrote:
>> > A function with the LLVM version checked is moved to the top.
>> > The function is called where the old code was.
>>
>> Replace the second line with "... in order to reuse/rework X"
>>
>> > No functional change.
>> >
>> > Signed-off-by: Tobias Droste 
>> > ---
>> >
>> >  configure.ac | 91
>> >   1 file
>> >  changed, 49 insertions(+), 42 deletions(-)
>> >
>> > diff --git a/configure.ac b/configure.ac
>> > index 933e7b5..0f19a4f 100644
>> > --- a/configure.ac
>> > +++ b/configure.ac
>> > @@ -861,6 +861,54 @@ fi
>> >
>> >  AC_SUBST([SELINUX_CFLAGS])
>> >  AC_SUBST([SELINUX_LIBS])
>> >
>> > +dnl
>> > +dnl LLVM
>> > +dnl
>> > +llvm_get_version() {
>>
>> Nit: The code below does a lot more than "get_version" -
>> get_environment/set_variables/other
>
> Any ideas? :-D
>
There's a couple above - I'm leaning towards [llvm_]get_environment
but feel free to use something else.

>>
>> With the above
>> Reviewed-by: Emil Velikov 
>>
>> > +if test -z "${LLVM_CONFIG}"; then
>> > +if test -n "$llvm_prefix"; then
>> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no],
>> > ["$llvm_prefix/bin"]) +else
>> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
>> > +fi
>> > +fi
>> > +
>> > +if test "x$LLVM_CONFIG" != xno; then
>> > +LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
>>
>> ...
>>
>> > +else
>> > +MESA_LLVM=0
>> > +LLVM_VERSION_INT=0
>>
>> Just realised that we should error out in this case. After all one
>> requests llvm, so silently ignoring that they're missing llvm-config
>> isn't a smart idea. Something like below (be that as a preparatory,
>> in-between or at the end of the series) would be great.
>
> At this point in time we don't know if we actually need LLVM.
Looking at the code in master (and at this point in your series) I see
no way how this can happen.
Can you point out where/how we can get that ?

Either way... this is in the "follow-up" ideas category.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 08/25] configure.ac: Move LLVM version check to the top

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 10:11:50 CEST schrieb Emil Velikov:
> On 12 October 2016 at 00:02, Tobias Droste  wrote:
> > A function with the LLVM version checked is moved to the top.
> > The function is called where the old code was.
> 
> Replace the second line with "... in order to reuse/rework X"
> 
> > No functional change.
> > 
> > Signed-off-by: Tobias Droste 
> > ---
> > 
> >  configure.ac | 91
> >   1 file
> >  changed, 49 insertions(+), 42 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 933e7b5..0f19a4f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -861,6 +861,54 @@ fi
> > 
> >  AC_SUBST([SELINUX_CFLAGS])
> >  AC_SUBST([SELINUX_LIBS])
> > 
> > +dnl
> > +dnl LLVM
> > +dnl
> > +llvm_get_version() {
> 
> Nit: The code below does a lot more than "get_version" -
> get_environment/set_variables/other

Any ideas? :-D

> 
> With the above
> Reviewed-by: Emil Velikov 
> 
> > +if test -z "${LLVM_CONFIG}"; then
> > +if test -n "$llvm_prefix"; then
> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no],
> > ["$llvm_prefix/bin"]) +else
> > +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
> > +fi
> > +fi
> > +
> > +if test "x$LLVM_CONFIG" != xno; then
> > +LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
> 
> ...
> 
> > +else
> > +MESA_LLVM=0
> > +LLVM_VERSION_INT=0
> 
> Just realised that we should error out in this case. After all one
> requests llvm, so silently ignoring that they're missing llvm-config
> isn't a smart idea. Something like below (be that as a preparatory,
> in-between or at the end of the series) would be great.

At this point in time we don't know if we actually need LLVM.
This will be handled by all the llvm_check_version_for() calls for each 
driver. 
This has also the advantage, that you not only know that you need LLVM but you 
also know which version!

> 
> if test "x$LLVM_CONFIG" = xno; then
>AC_MSG_ERROR([...])
> fi
> 
> LLVM_VERSION=...
> ...
> 
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 08/25] configure.ac: Move LLVM version check to the top

[Emil Velikov]

On 12 October 2016 at 00:02, Tobias Droste  wrote:
> A function with the LLVM version checked is moved to the top.
> The function is called where the old code was.
Replace the second line with "... in order to reuse/rework X"

> No functional change.
>
> Signed-off-by: Tobias Droste 
> ---
>  configure.ac | 91 
> 
>  1 file changed, 49 insertions(+), 42 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 933e7b5..0f19a4f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -861,6 +861,54 @@ fi
>  AC_SUBST([SELINUX_CFLAGS])
>  AC_SUBST([SELINUX_LIBS])
>
> +dnl
> +dnl LLVM
> +dnl
> +llvm_get_version() {
Nit: The code below does a lot more than "get_version" -
get_environment/set_variables/other

With the above
Reviewed-by: Emil Velikov 

> +if test -z "${LLVM_CONFIG}"; then
> +if test -n "$llvm_prefix"; then
> +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no], 
> ["$llvm_prefix/bin"])
> +else
> +AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
> +fi
> +fi
> +
> +if test "x$LLVM_CONFIG" != xno; then
> +LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
...
> +else
> +MESA_LLVM=0
> +LLVM_VERSION_INT=0
Just realised that we should error out in this case. After all one
requests llvm, so silently ignoring that they're missing llvm-config
isn't a smart idea. Something like below (be that as a preparatory,
in-between or at the end of the series) would be great.

if test "x$LLVM_CONFIG" = xno; then
   AC_MSG_ERROR([...])
fi

LLVM_VERSION=...
...

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3 08/25] configure.ac: Move LLVM version check to the top

[Tobias Droste]

A function with the LLVM version checked is moved to the top.
The function is called where the old code was.
No functional change.

Signed-off-by: Tobias Droste 
---
 configure.ac | 91 
 1 file changed, 49 insertions(+), 42 deletions(-)

diff --git a/configure.ac b/configure.ac
index 933e7b5..0f19a4f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -861,6 +861,54 @@ fi
 AC_SUBST([SELINUX_CFLAGS])
 AC_SUBST([SELINUX_LIBS])
 
+dnl
+dnl LLVM
+dnl
+llvm_get_version() {
+if test -z "${LLVM_CONFIG}"; then
+if test -n "$llvm_prefix"; then
+AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no], 
["$llvm_prefix/bin"])
+else
+AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
+fi
+fi
+
+if test "x$LLVM_CONFIG" != xno; then
+LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
+LLVM_LDFLAGS=`$LLVM_CONFIG --ldflags`
+LLVM_BINDIR=`$LLVM_CONFIG --bindir`
+LLVM_CPPFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cppflags"`
+LLVM_CFLAGS=$LLVM_CPPFLAGS   # CPPFLAGS seem to be sufficient
+LLVM_CXXFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cxxflags"`
+LLVM_INCLUDEDIR=`$LLVM_CONFIG --includedir`
+LLVM_LIBDIR=`$LLVM_CONFIG --libdir`
+
+AC_COMPUTE_INT([LLVM_VERSION_MAJOR], [LLVM_VERSION_MAJOR],
+[#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"])
+
+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'`
+fi
+
+llvm_add_default_components
+
+DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT 
-DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH"
+MESA_LLVM=1
+else
+MESA_LLVM=0
+LLVM_VERSION_INT=0
+fi
+}
+
 dnl Options for APIs
 AC_ARG_ENABLE([opengl],
 [AS_HELP_STRING([--disable-opengl],
@@ -2242,48 +2290,7 @@ if test "x$enable_gallium_llvm" = xauto; then
 esac
 fi
 if test "x$enable_gallium_llvm" = xyes || test "x$HAVE_RADEON_VULKAN" = xyes; 
then
-if test -z "${LLVM_CONFIG}"; then
-if test -n "$llvm_prefix"; then
-AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no], 
["$llvm_prefix/bin"])
-else
-AC_PATH_TOOL([LLVM_CONFIG], [llvm-config], [no])
-fi
-fi
-
-if test "x$LLVM_CONFIG" != xno; then
-LLVM_VERSION=`$LLVM_CONFIG --version | egrep -o '^[[0-9.]]+'`
-LLVM_LDFLAGS=`$LLVM_CONFIG --ldflags`
-LLVM_BINDIR=`$LLVM_CONFIG --bindir`
-LLVM_CPPFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cppflags"`
-LLVM_CFLAGS=$LLVM_CPPFLAGS   # CPPFLAGS seem to be sufficient
-LLVM_CXXFLAGS=`strip_unwanted_llvm_flags "$LLVM_CONFIG --cxxflags"`
-LLVM_INCLUDEDIR=`$LLVM_CONFIG --includedir`
-LLVM_LIBDIR=`$LLVM_CONFIG --libdir`
-
-AC_COMPUTE_INT([LLVM_VERSION_MAJOR], [LLVM_VERSION_MAJOR],
-[#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"])
-
-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'`
-fi
-
-llvm_add_default_components
-
-DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT 
-DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH"
-MESA_LLVM=1
-else
-MESA_LLVM=0
-LLVM_VERSION_INT=0
-fi
+llvm_get_version
 else
 MESA_LLVM=0
 LLVM_VERSION_INT=0
-- 
2.10.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev