Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 20:28:03 CEST schrieb Emil Velikov:
> On 12 October 2016 at 19:58, Tobias Droste  wrote:
> > Am Mittwoch, 12. Oktober 2016, 19:51:21 CEST schrieb Emil Velikov:
> >> On 12 October 2016 at 19:04, Tobias Droste  wrote:
> >> > Am Mittwoch, 12. Oktober 2016, 09:56:39 CEST schrieb Emil Velikov:
> >> >> >  fi
> >> >> > 
> >> >> > +if test "x$enable_opencl" = xyes; then
> >> >> > +llvm_check_version_for "3" "6" "0" "opencl"
> >> >> > +
> >> >> > +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
> >> >> > instrumentation" +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader
> >> >> > option
> >> >> > objcarcopts profiledata" +fi
> >> >> > +
> >> >> > +dnl Check for Clang internal headers
> >> >> > +if test "x$enable_opencl" = xyes; then
> >> >> 
> >> >> Nit: drop the second if test, yet preserve the comment ?
> >> >> Disclaimer: haven't looked if later patches depend on the split.
> >> > 
> >> > This is a "just move" patch, that's why I didn't change anything.
> >> > But this whole section will be changed later (Patch 11) so it actually
> >> > doesn't matter.
> >> 
> >> Patch 11... this hunks get moved once here and a second time in there.
> >> Just move it to the "top" from the start, fold the conditional and as
> >> you do further movement keep it in the same block ?
> >> 
> >> Sure I suggested to keep things separate, but it sounds like we got
> >> from one end/extreme to the other.
> >> Emil
> > 
> > No at this point in time it sadly has to be below the other gallium stuff,
> > because it uses LLVM variables but I need it outside the LLVM config stuff
> > to have this function later without any code that throws errors.
> > 
> > (Forgot the mailing list again)
> > 
> > PS:
> > This is the reason I didn't want to split this stuff and have it build
> > correctly. There's a lot of stuff that needs to be in the right order to
> > work. That's also the reason the oCL stuff is here and not where it
> > actually belongs!
> 
> In general - divide and concur. If the latter isn't working out the
> former needs refinement.
> 
> I'm starting to wonder if I shouldn't give it a bash myself rather
> than nitpicking like an old bat ?

I don't mind the feedback, so no worries there. But feel free to do so :-)
You can take whatever you want from my series and try to make it look better.

I just want to say that it looks easier than it actually is ;-)
Btw. some of the patches are also 2 patches instead of 1, because git 
sometimes isn't really that good in providing understandable diffs.

If you actually do try, give me a hint, so I can stop trying :-D

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

Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Emil Velikov]

On 12 October 2016 at 19:58, Tobias Droste  wrote:
> Am Mittwoch, 12. Oktober 2016, 19:51:21 CEST schrieb Emil Velikov:
>> On 12 October 2016 at 19:04, Tobias Droste  wrote:
>> > Am Mittwoch, 12. Oktober 2016, 09:56:39 CEST schrieb Emil Velikov:
>> >> >  fi
>> >> >
>> >> > +if test "x$enable_opencl" = xyes; then
>> >> > +llvm_check_version_for "3" "6" "0" "opencl"
>> >> > +
>> >> > +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
>> >> > instrumentation" +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader
>> >> > option
>> >> > objcarcopts profiledata" +fi
>> >> > +
>> >> > +dnl Check for Clang internal headers
>> >> > +if test "x$enable_opencl" = xyes; then
>> >>
>> >> Nit: drop the second if test, yet preserve the comment ?
>> >> Disclaimer: haven't looked if later patches depend on the split.
>> >
>> > This is a "just move" patch, that's why I didn't change anything.
>> > But this whole section will be changed later (Patch 11) so it actually
>> > doesn't matter.
>>
>> Patch 11... this hunks get moved once here and a second time in there.
>> Just move it to the "top" from the start, fold the conditional and as
>> you do further movement keep it in the same block ?
>>
>> Sure I suggested to keep things separate, but it sounds like we got
>> from one end/extreme to the other.
>> Emil
>
> No at this point in time it sadly has to be below the other gallium stuff,
> because it uses LLVM variables but I need it outside the LLVM config stuff to
> have this function later without any code that throws errors.
>
> (Forgot the mailing list again)
>
> PS:
> This is the reason I didn't want to split this stuff and have it build
> correctly. There's a lot of stuff that needs to be in the right order to work.
> That's also the reason the oCL stuff is here and not where it actually
> belongs!
>
In general - divide and concur. If the latter isn't working out the
former needs refinement.

I'm starting to wonder if I shouldn't give it a bash myself rather
than nitpicking like an old bat ?

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

Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 19:51:21 CEST schrieb Emil Velikov:
> On 12 October 2016 at 19:04, Tobias Droste  wrote:
> > Am Mittwoch, 12. Oktober 2016, 09:56:39 CEST schrieb Emil Velikov:
> >> >  fi
> >> > 
> >> > +if test "x$enable_opencl" = xyes; then
> >> > +llvm_check_version_for "3" "6" "0" "opencl"
> >> > +
> >> > +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
> >> > instrumentation" +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader
> >> > option
> >> > objcarcopts profiledata" +fi
> >> > +
> >> > +dnl Check for Clang internal headers
> >> > +if test "x$enable_opencl" = xyes; then
> >> 
> >> Nit: drop the second if test, yet preserve the comment ?
> >> Disclaimer: haven't looked if later patches depend on the split.
> > 
> > This is a "just move" patch, that's why I didn't change anything.
> > But this whole section will be changed later (Patch 11) so it actually
> > doesn't matter.
> 
> Patch 11... this hunks get moved once here and a second time in there.
> Just move it to the "top" from the start, fold the conditional and as
> you do further movement keep it in the same block ?
> 
> Sure I suggested to keep things separate, but it sounds like we got
> from one end/extreme to the other.
> Emil

No at this point in time it sadly has to be below the other gallium stuff, 
because it uses LLVM variables but I need it outside the LLVM config stuff to 
have this function later without any code that throws errors.

(Forgot the mailing list again)

PS:
This is the reason I didn't want to split this stuff and have it build 
correctly. There's a lot of stuff that needs to be in the right order to work. 
That's also the reason the oCL stuff is here and not where it actually 
belongs!

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

Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Emil Velikov]

On 12 October 2016 at 19:04, Tobias Droste  wrote:
> Am Mittwoch, 12. Oktober 2016, 09:56:39 CEST schrieb Emil Velikov:
>> >  fi
>> >
>> > +if test "x$enable_opencl" = xyes; then
>> > +llvm_check_version_for "3" "6" "0" "opencl"
>> > +
>> > +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
>> > instrumentation" +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader option
>> > objcarcopts profiledata" +fi
>> > +
>> > +dnl Check for Clang internal headers
>> > +if test "x$enable_opencl" = xyes; then
>>
>> Nit: drop the second if test, yet preserve the comment ?
>> Disclaimer: haven't looked if later patches depend on the split.
>
> This is a "just move" patch, that's why I didn't change anything.
> But this whole section will be changed later (Patch 11) so it actually doesn't
> matter.
>
Patch 11... this hunks get moved once here and a second time in there.
Just move it to the "top" from the start, fold the conditional and as
you do further movement keep it in the same block ?

Sure I suggested to keep things separate, but it sounds like we got
from one end/extreme to the other.
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Tobias Droste]

Am Mittwoch, 12. Oktober 2016, 09:56:39 CEST schrieb Emil Velikov:
> >  fi
> > 
> > +if test "x$enable_opencl" = xyes; then
> > +llvm_check_version_for "3" "6" "0" "opencl"
> > +
> > +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker
> > instrumentation" +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader option
> > objcarcopts profiledata" +fi
> > +
> > +dnl Check for Clang internal headers
> > +if test "x$enable_opencl" = xyes; then
> 
> Nit: drop the second if test, yet preserve the comment ?
> Disclaimer: haven't looked if later patches depend on the split.

This is a "just move" patch, that's why I didn't change anything.
But this whole section will be changed later (Patch 11) so it actually doesn't 
matter.

I leave it as is but keep your reviewed-by, ok? :-)

> 
> With the above
> Reviewed-by: Emil Velikov 
> 
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 04/25] configure.ac: Move oCL checks out of LLVM version check

[Emil Velikov]

On 12 October 2016 at 00:02, Tobias Droste  wrote:
> The openCL checks don't need to be inside the LLVM version check.
> "llvm_check_version_for" also works if LLVM wasn't found.
>
> Signed-off-by: Tobias Droste 
> ---
>  configure.ac | 33 +
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index e48ed57..dcd59fc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2284,24 +2284,8 @@ if test "x$enable_gallium_llvm" = xyes || test 
> "x$HAVE_RADEON_VULKAN" = xyes; th
>
>  llvm_add_default_components
>
> -if test "x$enable_opencl" = xyes; then
> -llvm_check_version_for "3" "6" "0" "opencl"
> -
> -LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker 
> instrumentation"
> -LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader option objcarcopts 
> profiledata"
> -fi
>  DEFINES="${DEFINES} -DHAVE_LLVM=0x0$LLVM_VERSION_INT 
> -DMESA_LLVM_VERSION_PATCH=$LLVM_VERSION_PATCH"
>  MESA_LLVM=1
> -
> -dnl Check for Clang internal headers
> -if test "x$enable_opencl" = xyes; then
> -if test -z "$CLANG_LIBDIR"; then
> -CLANG_LIBDIR=${LLVM_LIBDIR}
> -fi
> -CLANG_RESOURCE_DIR=$CLANG_LIBDIR/clang/${LLVM_VERSION}
> -AS_IF([test ! -f "$CLANG_RESOURCE_DIR/include/stddef.h"],
> -[AC_MSG_ERROR([Could not find clang internal header stddef.h 
> in $CLANG_RESOURCE_DIR Use --with-clang-libdir to specify the correct path to 
> the clang libraries.])])
> -fi
>  else
>  MESA_LLVM=0
>  LLVM_VERSION_INT=0
> @@ -2315,6 +2299,23 @@ else
>  fi
>  fi
>
> +if test "x$enable_opencl" = xyes; then
> +llvm_check_version_for "3" "6" "0" "opencl"
> +
> +LLVM_COMPONENTS="${LLVM_COMPONENTS} all-targets ipo linker 
> instrumentation"
> +LLVM_COMPONENTS="${LLVM_COMPONENTS} irreader option objcarcopts 
> profiledata"
> +fi
> +
> +dnl Check for Clang internal headers
> +if test "x$enable_opencl" = xyes; then
Nit: drop the second if test, yet preserve the comment ?
Disclaimer: haven't looked if later patches depend on the split.

With the above
Reviewed-by: Emil Velikov 

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