add support for building spir-v support library from libclc

This series adds support to libclc to produce a spir-v library for
most of the libclc functions. This library is to be consumed by the
Mesa SPIR-V backend in order to allow drivers to pick which libclc
implementations vs native ones they want to choose.

This library will be converted in Mesa into NIR and linked with the
CL kernel. It allows Mesa to use libclc to implement complex functions
using CLC rather than directly in IR. We don't include all the libclc
functions, some of them have no SPIR-V representation, and some of them
are generate a lot more code than just writing the IR in Mesa (e.g.
shuffle* added 10MB to this file for 30 lines of IR generator).

I'm sure we'll tweak over time what goes into the library, but this works
for most of the built-ins piglit tests at the moment.

Dave.

We need to investigate how this is going to work in a SPIR-V world

This adds the ability to create two new targets (spirv--.spv and spirv64--.spv)
that contain the SPIR-V builds of the major libclc components.

It uses a separate sources file as it doesn't require any of the sources
that directly wrap llvm intrinsics or use __builtin intrinsics.

It builds unoptimised bytecode as the llvm -> spirv translator requires it.
It inlines only some functions, but in general we don't want inlining as
it makes the binary quite large.

v2: drop shuffle. add vload/store_half variants

This should be unconditionally true. I don't know why this function even exists; they're not optional. It's also allowed to interpret the cl-fp-denorms-are-zero as only impacting f32.

-Matt

This should be unconditionally true. I don't know why this function even exists; they're not optional. It's also allowed to interpret the cl-fp-denorms-are-zero as only impacting f32.

-Matt

    We need to investigate how this is going to work in a SPIR-V world
    ---
     libclc/generic/lib/subnormal_config.cl | 4 ++++
     1 file changed, 4 insertions(+)
    
    diff --git a/libclc/generic/lib/subnormal_config.cl b/libclc/generic/lib/subnormal_config.cl
    index 4bcecfd82e1..2a754db8317 100644
    --- a/libclc/generic/lib/subnormal_config.cl
    +++ b/libclc/generic/lib/subnormal_config.cl
    @@ -33,5 +33,9 @@ _CLC_DEF bool __clc_fp32_subnormals_supported() {
     }
    
     _CLC_DEF bool __clc_fp64_subnormals_supported() {
    +#ifdef CLC_SPIRV //TODO workout how to support this in SPIR-V land
    + return false;
    +#else
       return !__clc_subnormals_disabled();
    +#endif
     }

this function is not used anywhere and can be removed.

Jan

Is it illegal to call step/smoothstep when targetting SPIR-V? or do
the headers implement it as a macro?
either way it'd be preferable to follow the existing overload
mechanism rather than adding ifdefs across the sources (more in
reponse to the cmake changes).

Jan

the SPIR-V spec doesn't define step/smoothstep with those interfaces,
it des define the others that are left unifdef, whether that is a
mistake or not I'm not sure.

If the SPIR-V spec never defines those APIs then no code will want to
access them in the libclc spir-v library.

I'm not sure how the exisiting overload mechanism can remove these
from the library completely, create empty impls?

Dave.

> > ---
> > libclc/generic/lib/common/smoothstep.cl | 3 ++-
> > libclc/generic/lib/common/step.cl | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/libclc/generic/lib/common/smoothstep.cl b/libclc/generic/lib/common/smoothstep.cl
> > index 68d1a13ab39..9ff41e13291 100644
> > --- a/libclc/generic/lib/common/smoothstep.cl
> > +++ b/libclc/generic/lib/common/smoothstep.cl
> > @@ -46,10 +46,11 @@ SMOOTH_STEP_DEF(double, double, SMOOTH_STEP_IMPL_D);
> >
> > _CLC_TERNARY_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, double, smoothstep, double, double, double);
> >
> > +#ifndef CLC_SPIRV // SPIRV doesn't define these
> > SMOOTH_STEP_DEF(float, double, SMOOTH_STEP_IMPL_D);
> > SMOOTH_STEP_DEF(double, float, SMOOTH_STEP_IMPL_D);
> >
> > _CLC_V_S_S_V_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, double, smoothstep, float, float, double);
> > _CLC_V_S_S_V_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, float, smoothstep, double, double, float);
> > -
> > +#endif
> > #endif
> > diff --git a/libclc/generic/lib/common/step.cl b/libclc/generic/lib/common/step.cl
> > index 4b022f1316c..3ceec7b8be3 100644
> > --- a/libclc/generic/lib/common/step.cl
> > +++ b/libclc/generic/lib/common/step.cl
> > @@ -45,10 +45,11 @@ STEP_DEF(double, double);
> > _CLC_BINARY_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, double, step, double, double);
> > _CLC_V_S_V_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, double, step, double, double);
> >
> > +#ifndef CLC_SPIRV
> > STEP_DEF(float, double);
> > STEP_DEF(double, float);
> >
> > _CLC_V_S_V_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, double, step, float, double);
> > _CLC_V_S_V_VECTORIZE(_CLC_OVERLOAD _CLC_DEF, float, step, double, float);
> > -
> > +#endif
> > #endif
> Is it illegal to call step/smoothstep when targetting SPIR-V? or do
> the headers implement it as a macro?
> either way it'd be preferable to follow the existing overload
> mechanism rather than adding ifdefs across the sources (more in
> reponse to the cmake changes).

the SPIR-V spec doesn't define step/smoothstep with those interfaces,
it des define the others that are left unifdef, whether that is a
mistake or not I'm not sure.

If the SPIR-V spec never defines those APIs then no code will want to
access them in the libclc spir-v library.

ah right, you'll be using it SPIR-V runtime library rather than CLC.
makes sense.

I'm not sure how the exisiting overload mechanism can remove these
from the library completely, create empty impls?

right, I forgot I didn't port the OVERRIDES mechanism to cmake. ifdefs
are nicer than empty files.

Jan

This adds the ability to create two new targets (spirv--.spv and spirv64--.spv)
that contain the SPIR-V builds of the major libclc components.

It uses a separate sources file as it doesn't require any of the sources
that directly wrap llvm intrinsics or use __builtin intrinsics.

It builds unoptimised bytecode as the llvm -> spirv translator requires it.
It inlines only some functions, but in general we don't want inlining as
it makes the binary quite large.

v2: drop shuffle. add vload/store_half variants
---
libclc/CMakeLists.txt | 67 +++++++++++++++++++++---
libclc/generic/lib/SOURCES-SPIRV | 82 ++++++++++++++++++++++++++++++
libclc/generic/lib/shared/vload.cl | 3 +-
3 files changed, 143 insertions(+), 9 deletions(-)
create mode 100644 libclc/generic/lib/SOURCES-SPIRV

diff --git a/libclc/CMakeLists.txt b/libclc/CMakeLists.txt
index 440eab07650..7af3eaa51ee 100644
--- a/libclc/CMakeLists.txt
+++ b/libclc/CMakeLists.txt
@@ -39,6 +39,13 @@ if( ${LLVM_VERSION} VERSION_GREATER "3.9.0" )
   set( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} amdgcn-mesa-mesa3d )
endif()

+find_program( LLVM_SPIRV llvm-spirv PATHS ${LLVM_BINDIR} )
+if( NOT LLVM_SPIRV )
+ message( "libclc won't build spir-v support." )
+else()
+ set ( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} spirv-- spirv64-- )
+endif()

No automagic dependencies please. The build should fail if spirv was
requested and llvm-spirv not found.

+
if( LIBCLC_TARGETS_TO_BUILD STREQUAL "all" )
   set( LIBCLC_TARGETS_TO_BUILD ${LIBCLC_TARGETS_ALL} )
endif()
@@ -81,12 +88,14 @@ find_program( LLVM_CLANG clang PATHS ${LLVM_BINDIR} NO_DEFAULT_PATH )
find_program( LLVM_AS llvm-as PATHS ${LLVM_BINDIR} NO_DEFAULT_PATH )
find_program( LLVM_LINK llvm-link PATHS ${LLVM_BINDIR} NO_DEFAULT_PATH )
find_program( LLVM_OPT opt PATHS ${LLVM_BINDIR} NO_DEFAULT_PATH )
+find_program( LLVM_SPIRV llvm-spirv PATHS ${LLVM_BINDIR} )

# Print toolchain
message( "clang: ${LLVM_CLANG}" )
message( "llvm-as: ${LLVM_AS}" )
message( "llvm-link: ${LLVM_LINK}" )
message( "opt: ${LLVM_OPT}" )
+message( "llvm-spirv: ${LLVM_SPIRV}" )
message( "" )
if( NOT LLVM_CLANG OR NOT LLVM_OPT OR NOT LLVM_AS OR NOT LLVM_LINK )
   message( FATAL_ERROR "toolchain incomplete!" )
@@ -125,6 +134,10 @@ set( nvptx--_devices none )
set( nvptx64--_devices none )
set( nvptx--nvidiacl_devices none )
set( nvptx64--nvidiacl_devices none )
+if ( LLVM_SPIRV )
+ set( spirv--_devices none )
+ set( spirv64--_devices none )
+endif()

I don't think there needs to be a conditional here.

# Setup aliases
set( cedar_aliases palm sumo sumo2 redwood juniper )
@@ -187,10 +200,14 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
     set( DARCH ${ARCH} )
   endif()

+ set( sourcefilename "SOURCES" )
+ if ( ${ARCH} STREQUAL spirv OR ${ARCH} STREQUAL spirv64 )
+ set( sourcefilename "SOURCES-SPIRV" )
+ endif()

I haven't gone through the details. How many files from SOURCES are
not present in SOURCES_SPIRV?

   # Enumerate SOURCES* files
   set( source_list )
   foreach( l ${dirs} ${DARCH} ${DARCH}-${OS} ${DARCH}-${VENDOR}-${OS} )
- foreach( s "SOURCES" "SOURCES_${LLVM_MAJOR}.${LLVM_MINOR}" )
+ foreach( s "${sourcefilename}" "${sourcefilename}_${LLVM_MAJOR}.${LLVM_MINOR}" )
       file( TO_CMAKE_PATH ${l}/lib/${s} file_loc )
       file( TO_CMAKE_PATH ${CMAKE_SOURCE_DIR}/${file_loc} loc )
       # Prepend the location to give higher priority to
@@ -203,10 +220,15 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )

   # Add the generated convert.cl here to prevent adding
   # the one listed in SOURCES
- set( rel_files convert.cl )
- set( objects convert.cl )
- if( NOT ENABLE_RUNTIME_SUBNORMAL )
- list( APPEND rel_files generic/lib/subnormal_use_default.ll )
+ if ( ${ARCH} STREQUAL spirv OR ${ARCH} STREQUAL spirv64 )
+ set ( rel_files )
+ set ( objects )
+ else()
+ set( rel_files convert.cl )
+ set( objects convert.cl )
+ if( NOT ENABLE_RUNTIME_SUBNORMAL )
+ list( APPEND rel_files generic/lib/subnormal_use_default.ll )
+ endif()

ENABLE_RUNTIME_SUBNORMAL should produce an error if spirv is the only
target.

   endif()

   foreach( l ${source_list} )
@@ -229,6 +251,20 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
   endforeach()

   foreach( d ${${t}_devices} )
+
+ set (dflags "")
+ set (oflags "")
+ if ( ${t} STREQUAL "spirv--" )
+ set (target "spir--")
+ set (dflags "-DCLC_SPIRV")
+ set (oflags -O0 -finline-hint-functions)
+ elseif ( ${t} STREQUAL "spirv64--" )
+ set (target "spir64--")
+ set (dflags "-DCLC_SPIRV")
+ set (oflags -O0 -finline-hint-functions)
+ else()
+ set (target ${t})
+ endif()
     # Some targets don't have a specific GPU to target
     if( ${d} STREQUAL "none" )
       set( mcpu )
@@ -250,11 +286,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
     target_compile_definitions( builtins.link.${arch_suffix} PRIVATE
       "__CLC_INTERNAL" )
     target_compile_options( builtins.link.${arch_suffix} PRIVATE -target
- ${t} ${mcpu} -fno-builtin )
+ ${target} ${mcpu} -fno-builtin ${oflags} ${dflags} )

definitions should be handled in the target_compile_defitions above.
I think we can just add generic LIBCLC_TARGET_${t}, alternatively
clang defines __SPIR32__ and __SPIR64__ when targetting spir so we
don't really need a custom define.

Why -O0 necessary? is that not clang default?
-finline-hint-functions can be probably enabled for all targets, but I
need to check if it includes the always-inliner pass.

     set_target_properties( builtins.link.${arch_suffix} PROPERTIES
       LINKER_LANGUAGE CLC )

     set( obj_suffix ${arch_suffix}.bc )
+ set( spv_suffix ${arch_suffix}.spv )

     # Add opt target
     add_custom_command( OUTPUT "builtins.opt.${obj_suffix}"
@@ -265,6 +302,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
     add_custom_target( "opt.${obj_suffix}" ALL
                        DEPENDS "builtins.opt.${obj_suffix}" )

+ add_custom_command( OUTPUT "${spv_suffix}"
+ COMMAND ${LLVM_SPIRV} -o
+ "${spv_suffix}"
+ "builtins.link.${obj_suffix}"
+ DEPENDS "builtins.link.${arch_suffix}")
+
     # Add prepare target
     add_custom_command( OUTPUT "${obj_suffix}"
                   COMMAND prepare_builtins -o
@@ -274,8 +317,16 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
                     "builtins.opt.${obj_suffix}"
                     prepare_builtins )
     add_custom_target( "prepare-${obj_suffix}" ALL
- DEPENDS "${obj_suffix}" )
- install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
+ DEPENDS "${obj_suffix}" )
+
+ if ( ${t} STREQUAL "spirv--" OR ${t} STREQUAL "spirv64--")
+ # Add spv target
+ add_custom_target( "spv.${spv_suffix}" ALL
+ DEPENDS "${spv_suffix}" )
+ install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${spv_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
+ else()
+ install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
+ endif()
     # nvptx-- targets don't include workitem builtins
     if( NOT ${t} MATCHES ".*ptx.*--$" )
       add_test( NAME external-calls-${obj_suffix}
diff --git a/libclc/generic/lib/SOURCES-SPIRV b/libclc/generic/lib/SOURCES-SPIRV
new file mode 100644
index 00000000000..a3257a932bd
--- /dev/null
+++ b/libclc/generic/lib/SOURCES-SPIRV
@@ -0,0 +1,82 @@
+subnormal_config.cl
+common/degrees.cl
+common/mix.cl
+common/radians.cl
+common/sign.cl
+common/smoothstep.cl
+common/step.cl
+geometric/cross.cl
+geometric/distance.cl
+geometric/dot.cl
+geometric/fast_distance.cl
+geometric/fast_length.cl
+geometric/fast_normalize.cl
+geometric/length.cl
+geometric/normalize.cl
+integer/rotate.cl
+integer/mad_sat.cl
+math/acos.cl
+math/acosh.cl
+math/acospi.cl
+math/asin.cl
+math/asinh.cl
+math/asinpi.cl
+math/atan.cl
+math/atan2.cl
+math/atan2pi.cl
+math/atanh.cl
+math/atanpi.cl
+math/cbrt.cl
+math/cos.cl
+math/cosh.cl
+math/cospi.cl
+math/ep_log.cl
+math/erf.cl
+math/erfc.cl
+math/exp.cl
+math/exp_helper.cl
+math/expm1.cl
+math/exp2.cl
+math/clc_exp10.cl
+math/exp10.cl
+math/fract.cl
+math/frexp.cl
+math/half_rsqrt.cl
+math/half_sqrt.cl
+math/clc_hypot.cl
+math/hypot.cl
+math/ilogb.cl
+math/lgamma.cl
+math/lgamma_r.cl
+math/log.cl
+math/log10.cl
+math/log1p.cl
+math/log2.cl
+math/logb.cl
+math/modf.cl
+math/tables.cl
+math/clc_pow.cl
+math/pow.cl
+math/clc_pown.cl
+math/pown.cl
+math/clc_powr.cl
+math/powr.cl
+math/clc_remainder.cl
+math/remainder.cl
+math/clc_remquo.cl
+math/remquo.cl
+math/clc_rootn.cl
+math/rootn.cl
+math/sin.cl
+math/sincos.cl
+math/sincos_helpers.cl
+math/sinh.cl
+math/sinpi.cl
+math/clc_tan.cl
+math/tan.cl
+math/tanh.cl
+math/clc_tanpi.cl
+math/tanpi.cl
+math/tgamma.cl
+shared/vload.cl
+shared/vstore.cl
diff --git a/libclc/generic/lib/shared/vload.cl b/libclc/generic/lib/shared/vload.cl
index 9c37fcf9981..9d28aac10c0 100644
--- a/libclc/generic/lib/shared/vload.cl
+++ b/libclc/generic/lib/shared/vload.cl
@@ -33,6 +33,7 @@
     VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __constant) \
     VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __global) \

+#ifndef __CLC_SPIRV__
#define VLOAD_TYPES() \
     VLOAD_ADDR_SPACES(char) \
     VLOAD_ADDR_SPACES(uchar) \
@@ -54,7 +55,7 @@ VLOAD_TYPES()
#pragma OPENCL EXTENSION cl_khr_fp16 : enable
     VLOAD_ADDR_SPACES(half)
#endif
-
+#endif

can you either split the .cl files changes, or include all of them?

I plan to take a more detailed look and actually build the library
sometime this week. I think there's a way to simplify couple of things
to make this integration easier, but it can always be cleaned up
later.

Jan

> This adds the ability to create two new targets (spirv--.spv and spirv64--.spv)
> that contain the SPIR-V builds of the major libclc components.
>
> It uses a separate sources file as it doesn't require any of the sources
> that directly wrap llvm intrinsics or use __builtin intrinsics.
>
> It builds unoptimised bytecode as the llvm -> spirv translator requires it.
> It inlines only some functions, but in general we don't want inlining as
> it makes the binary quite large.
>
> v2: drop shuffle. add vload/store_half variants
> ---
> libclc/CMakeLists.txt | 67 +++++++++++++++++++++---
> libclc/generic/lib/SOURCES-SPIRV | 82 ++++++++++++++++++++++++++++++
> libclc/generic/lib/shared/vload.cl | 3 +-
> 3 files changed, 143 insertions(+), 9 deletions(-)
> create mode 100644 libclc/generic/lib/SOURCES-SPIRV
>
> diff --git a/libclc/CMakeLists.txt b/libclc/CMakeLists.txt
> index 440eab07650..7af3eaa51ee 100644
> --- a/libclc/CMakeLists.txt
> +++ b/libclc/CMakeLists.txt
> @@ -39,6 +39,13 @@ if( ${LLVM_VERSION} VERSION_GREATER "3.9.0" )
> set( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} amdgcn-mesa-mesa3d )
> endif()
>
> +find_program( LLVM_SPIRV llvm-spirv PATHS ${LLVM_BINDIR} )
> +if( NOT LLVM_SPIRV )
> + message( "libclc won't build spir-v support." )
> +else()
> + set ( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} spirv-- spirv64-- )
> +endif()

No automagic dependencies please. The build should fail if spirv was
requested and llvm-spirv not found.

The question then is if we should have spirv generation be a separate
item rather than part of the "ALL" as otherwise people who don't
install the spirv tools won't be able to generate their current
configuations.

>
> # Print toolchain
> message( "clang: ${LLVM_CLANG}" )
> message( "llvm-as: ${LLVM_AS}" )
> message( "llvm-link: ${LLVM_LINK}" )
> message( "opt: ${LLVM_OPT}" )
> +message( "llvm-spirv: ${LLVM_SPIRV}" )
> message( "" )
> if( NOT LLVM_CLANG OR NOT LLVM_OPT OR NOT LLVM_AS OR NOT LLVM_LINK )
> message( FATAL_ERROR "toolchain incomplete!" )
> @@ -125,6 +134,10 @@ set( nvptx--_devices none )
> set( nvptx64--_devices none )
> set( nvptx--nvidiacl_devices none )
> set( nvptx64--nvidiacl_devices none )
> +if ( LLVM_SPIRV )
> + set( spirv--_devices none )
> + set( spirv64--_devices none )
> +endif()

I don't think there needs to be a conditional here.

>
> # Setup aliases
> set( cedar_aliases palm sumo sumo2 redwood juniper )
> @@ -187,10 +200,14 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> set( DARCH ${ARCH} )
> endif()
>
> + set( sourcefilename "SOURCES" )
> + if ( ${ARCH} STREQUAL spirv OR ${ARCH} STREQUAL spirv64 )
> + set( sourcefilename "SOURCES-SPIRV" )
> + endif()

I haven't gone through the details. How many files from SOURCES are
not present in SOURCES_SPIRV?

Lots, there are 82 in the spirv sources vs 219 in sources here. The
SPIR-V runtime library doesn't quite need a lot of the stuff that can
just be done in spir-v->nir.
There may be cases in the future to add some more bits, but it seems
fine with the current baseline.

> foreach( d ${${t}_devices} )
> +
> + set (dflags "")
> + set (oflags "")
> + if ( ${t} STREQUAL "spirv--" )
> + set (target "spir--")
> + set (dflags "-DCLC_SPIRV")
> + set (oflags -O0 -finline-hint-functions)
> + elseif ( ${t} STREQUAL "spirv64--" )
> + set (target "spir64--")
> + set (dflags "-DCLC_SPIRV")
> + set (oflags -O0 -finline-hint-functions)
> + else()
> + set (target ${t})
> + endif()
> # Some targets don't have a specific GPU to target
> if( ${d} STREQUAL "none" )
> set( mcpu )
> @@ -250,11 +286,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> target_compile_definitions( builtins.link.${arch_suffix} PRIVATE
> "__CLC_INTERNAL" )
> target_compile_options( builtins.link.${arch_suffix} PRIVATE -target
> - ${t} ${mcpu} -fno-builtin )
> + ${target} ${mcpu} -fno-builtin ${oflags} ${dflags} )
definitions should be handled in the target_compile_defitions above.
I think we can just add generic LIBCLC_TARGET_${t}, alternatively
clang defines __SPIR32__ and __SPIR64__ when targetting spir so we
don't really need a custom define.

Why -O0 necessary? is that not clang default?
-finline-hint-functions can be probably enabled for all targets, but I
need to check if it includes the always-inliner pass.

The LLVM->SPIRV converter only works on unoptimised LLVM IR, so we
have to pass -O0, but we want to inline any hinted functions.

My build here is defaulting clang to -O2 by the looks of it.

Do you suggest I change all the internal CLC_SPIRV to if
defined(__SPIR32__) || defined(__SPIR64__) ? might be a bit uglier but
it's not too many of them I suppose.

> set_target_properties( builtins.link.${arch_suffix} PROPERTIES
> LINKER_LANGUAGE CLC )
>
> set( obj_suffix ${arch_suffix}.bc )
> + set( spv_suffix ${arch_suffix}.spv )
>
> # Add opt target
> add_custom_command( OUTPUT "builtins.opt.${obj_suffix}"
> @@ -265,6 +302,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> add_custom_target( "opt.${obj_suffix}" ALL
> DEPENDS "builtins.opt.${obj_suffix}" )
>
> + add_custom_command( OUTPUT "${spv_suffix}"
> + COMMAND ${LLVM_SPIRV} -o
> + "${spv_suffix}"
> + "builtins.link.${obj_suffix}"
> + DEPENDS "builtins.link.${arch_suffix}")
> +
> # Add prepare target
> add_custom_command( OUTPUT "${obj_suffix}"
> COMMAND prepare_builtins -o
> @@ -274,8 +317,16 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> "builtins.opt.${obj_suffix}"
> prepare_builtins )
> add_custom_target( "prepare-${obj_suffix}" ALL
> - DEPENDS "${obj_suffix}" )
> - install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> + DEPENDS "${obj_suffix}" )
> +
> + if ( ${t} STREQUAL "spirv--" OR ${t} STREQUAL "spirv64--")
> + # Add spv target
> + add_custom_target( "spv.${spv_suffix}" ALL
> + DEPENDS "${spv_suffix}" )
> + install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${spv_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> + else()
> + install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> + endif()
> # nvptx-- targets don't include workitem builtins
> if( NOT ${t} MATCHES ".*ptx.*--$" )
> add_test( NAME external-calls-${obj_suffix}

> diff --git a/libclc/generic/lib/shared/vload.cl b/libclc/generic/lib/shared/vload.cl
> index 9c37fcf9981..9d28aac10c0 100644
> --- a/libclc/generic/lib/shared/vload.cl
> +++ b/libclc/generic/lib/shared/vload.cl
> @@ -33,6 +33,7 @@
> VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __constant) \
> VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __global) \
>
> +#ifndef __CLC_SPIRV__
> #define VLOAD_TYPES() \
> VLOAD_ADDR_SPACES(char) \
> VLOAD_ADDR_SPACES(uchar) \
> @@ -54,7 +55,7 @@ VLOAD_TYPES()
> #pragma OPENCL EXTENSION cl_khr_fp16 : enable
> VLOAD_ADDR_SPACES(half)
> #endif
> -
> +#endif

can you either split the .cl files changes, or include all of them?

Not sure what you mean, merge the previous ones into this patch or
split them all out? I think merging them makes more sense.

I plan to take a more detailed look and actually build the library
sometime this week. I think there's a way to simplify couple of things
to make this integration easier, but it can always be cleaned up
later.

I'm trying to rebase my mesa patches to use this onto master instead
of one of Karol's trees.

Thanks,
Dave.

> > This adds the ability to create two new targets (spirv--.spv and spirv64--.spv)
> > that contain the SPIR-V builds of the major libclc components.
> >
> > It uses a separate sources file as it doesn't require any of the sources
> > that directly wrap llvm intrinsics or use __builtin intrinsics.
> >
> > It builds unoptimised bytecode as the llvm -> spirv translator requires it.
> > It inlines only some functions, but in general we don't want inlining as
> > it makes the binary quite large.
> >
> > v2: drop shuffle. add vload/store_half variants
> > ---
> > libclc/CMakeLists.txt | 67 +++++++++++++++++++++---
> > libclc/generic/lib/SOURCES-SPIRV | 82 ++++++++++++++++++++++++++++++
> > libclc/generic/lib/shared/vload.cl | 3 +-
> > 3 files changed, 143 insertions(+), 9 deletions(-)
> > create mode 100644 libclc/generic/lib/SOURCES-SPIRV
> >
> > diff --git a/libclc/CMakeLists.txt b/libclc/CMakeLists.txt
> > index 440eab07650..7af3eaa51ee 100644
> > --- a/libclc/CMakeLists.txt
> > +++ b/libclc/CMakeLists.txt
> > @@ -39,6 +39,13 @@ if( ${LLVM_VERSION} VERSION_GREATER "3.9.0" )
> > set( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} amdgcn-mesa-mesa3d )
> > endif()
> >
> > +find_program( LLVM_SPIRV llvm-spirv PATHS ${LLVM_BINDIR} )
> > +if( NOT LLVM_SPIRV )
> > + message( "libclc won't build spir-v support." )
> > +else()
> > + set ( LIBCLC_TARGETS_ALL ${LIBCLC_TARGETS_ALL} spirv-- spirv64-- )
> > +endif()
>
> No automagic dependencies please. The build should fail if spirv was
> requested and llvm-spirv not found.

The question then is if we should have spirv generation be a separate
item rather than part of the "ALL" as otherwise people who don't
install the spirv tools won't be able to generate their current
configuations.

> > # Print toolchain
> > message( "clang: ${LLVM_CLANG}" )
> > message( "llvm-as: ${LLVM_AS}" )
> > message( "llvm-link: ${LLVM_LINK}" )
> > message( "opt: ${LLVM_OPT}" )
> > +message( "llvm-spirv: ${LLVM_SPIRV}" )
> > message( "" )
> > if( NOT LLVM_CLANG OR NOT LLVM_OPT OR NOT LLVM_AS OR NOT LLVM_LINK )
> > message( FATAL_ERROR "toolchain incomplete!" )
> > @@ -125,6 +134,10 @@ set( nvptx--_devices none )
> > set( nvptx64--_devices none )
> > set( nvptx--nvidiacl_devices none )
> > set( nvptx64--nvidiacl_devices none )
> > +if ( LLVM_SPIRV )
> > + set( spirv--_devices none )
> > + set( spirv64--_devices none )
> > +endif()
>
> I don't think there needs to be a conditional here.
> > # Setup aliases
> > set( cedar_aliases palm sumo sumo2 redwood juniper )
> > @@ -187,10 +200,14 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> > set( DARCH ${ARCH} )
> > endif()
> >
> > + set( sourcefilename "SOURCES" )
> > + if ( ${ARCH} STREQUAL spirv OR ${ARCH} STREQUAL spirv64 )
> > + set( sourcefilename "SOURCES-SPIRV" )
> > + endif()
>
> I haven't gone through the details. How many files from SOURCES are
> not present in SOURCES_SPIRV?

Lots, there are 82 in the spirv sources vs 219 in sources here. The
SPIR-V runtime library doesn't quite need a lot of the stuff that can
just be done in spir-v->nir.
There may be cases in the future to add some more bits, but it seems
fine with the current baseline.

OK, thanks.

> > foreach( d ${${t}_devices} )
> > +
> > + set (dflags "")
> > + set (oflags "")
> > + if ( ${t} STREQUAL "spirv--" )
> > + set (target "spir--")
> > + set (dflags "-DCLC_SPIRV")
> > + set (oflags -O0 -finline-hint-functions)
> > + elseif ( ${t} STREQUAL "spirv64--" )
> > + set (target "spir64--")
> > + set (dflags "-DCLC_SPIRV")
> > + set (oflags -O0 -finline-hint-functions)
> > + else()
> > + set (target ${t})
> > + endif()
> > # Some targets don't have a specific GPU to target
> > if( ${d} STREQUAL "none" )
> > set( mcpu )
> > @@ -250,11 +286,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> > target_compile_definitions( builtins.link.${arch_suffix} PRIVATE
> > "__CLC_INTERNAL" )
> > target_compile_options( builtins.link.${arch_suffix} PRIVATE -target
> > - ${t} ${mcpu} -fno-builtin )
> > + ${target} ${mcpu} -fno-builtin ${oflags} ${dflags} )
> definitions should be handled in the target_compile_defitions above.
> I think we can just add generic LIBCLC_TARGET_${t}, alternatively
> clang defines __SPIR32__ and __SPIR64__ when targetting spir so we
> don't really need a custom define.
>
> Why -O0 necessary? is that not clang default?
> -finline-hint-functions can be probably enabled for all targets, but I
> need to check if it includes the always-inliner pass.

The LLVM->SPIRV converter only works on unoptimised LLVM IR, so we
have to pass -O0, but we want to inline any hinted functions.

My build here is defaulting clang to -O2 by the looks of it.

Do you know which transformations break the conversion?

Do you suggest I change all the internal CLC_SPIRV to if
defined(__SPIR32__) || defined(__SPIR64__) ? might be a bit uglier but
it's not too many of them I suppose.

so far it has been used for __ADMGCN__ and __R600__, but since spirv
needs either both or none, I think it's OK to add a custom define.
As long as it's added in 'target_compile_definitions' instead to
'target_compile_options'.

> > set_target_properties( builtins.link.${arch_suffix} PROPERTIES
> > LINKER_LANGUAGE CLC )
> >
> > set( obj_suffix ${arch_suffix}.bc )
> > + set( spv_suffix ${arch_suffix}.spv )
> >
> > # Add opt target
> > add_custom_command( OUTPUT "builtins.opt.${obj_suffix}"
> > @@ -265,6 +302,12 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> > add_custom_target( "opt.${obj_suffix}" ALL
> > DEPENDS "builtins.opt.${obj_suffix}" )
> >
> > + add_custom_command( OUTPUT "${spv_suffix}"
> > + COMMAND ${LLVM_SPIRV} -o
> > + "${spv_suffix}"
> > + "builtins.link.${obj_suffix}"
> > + DEPENDS "builtins.link.${arch_suffix}")
> > +
> > # Add prepare target
> > add_custom_command( OUTPUT "${obj_suffix}"
> > COMMAND prepare_builtins -o
> > @@ -274,8 +317,16 @@ foreach( t ${LIBCLC_TARGETS_TO_BUILD} )
> > "builtins.opt.${obj_suffix}"
> > prepare_builtins )
> > add_custom_target( "prepare-${obj_suffix}" ALL
> > - DEPENDS "${obj_suffix}" )
> > - install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> > + DEPENDS "${obj_suffix}" )
> > +
> > + if ( ${t} STREQUAL "spirv--" OR ${t} STREQUAL "spirv64--")
> > + # Add spv target
> > + add_custom_target( "spv.${spv_suffix}" ALL
> > + DEPENDS "${spv_suffix}" )
> > + install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${spv_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> > + else()
> > + install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${obj_suffix} DESTINATION ${CMAKE_INSTALL_DATADIR}/clc )
> > + endif()
> > # nvptx-- targets don't include workitem builtins
> > if( NOT ${t} MATCHES ".*ptx.*--$" )
> > add_test( NAME external-calls-${obj_suffix}
> > diff --git a/libclc/generic/lib/shared/vload.cl b/libclc/generic/lib/shared/vload.cl
> > index 9c37fcf9981..9d28aac10c0 100644
> > --- a/libclc/generic/lib/shared/vload.cl
> > +++ b/libclc/generic/lib/shared/vload.cl
> > @@ -33,6 +33,7 @@
> > VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __constant) \
> > VLOAD_VECTORIZE(__CLC_SCALAR_GENTYPE, __global) \
> >
> > +#ifndef __CLC_SPIRV__
> > #define VLOAD_TYPES() \
> > VLOAD_ADDR_SPACES(char) \
> > VLOAD_ADDR_SPACES(uchar) \
> > @@ -54,7 +55,7 @@ VLOAD_TYPES()
> > #pragma OPENCL EXTENSION cl_khr_fp16 : enable
> > VLOAD_ADDR_SPACES(half)
> > #endif
> > -
> > +#endif
>
> can you either split the .cl files changes, or include all of them?

Not sure what you mean, merge the previous ones into this patch or
split them all out? I think merging them makes more sense.

I meant either splitting edits to *.cl to a separate patch or
including all of them with the build system change.

> I plan to take a more detailed look and actually build the library
> sometime this week. I think there's a way to simplify couple of things
> to make this integration easier, but it can always be cleaned up
> later.

I'm trying to rebase my mesa patches to use this onto master instead
of one of Karol's trees.

sorry for the delay. I planned to take a look last weekend but I spent
it converting all my test configurations to llvm monorepo.
I think the cmake changes might be cleaner if spirv just used a custom
path instead of trying co-opt the existing one, but I haven't
considered all the details and I don't want to delay these changes.

Tom is the maintainer so you might want to check with him too.
I'd ask for two changes mentioned:
1) use 'target_defines' instead of 'compile_options'
2) make configure fail if spirv target is requested and the converter
is not found

regards,
Jan