[Libclc-dev] [PATCH 4/4] spirv: add ability to build spirv from libclc
Jan Vesely via Libclc-dev
libclc-dev at lists.llvm.org
Mon Oct 28 11:11:55 PDT 2019
On Mon, 2019-10-14 at 14:10 +1000, David Airlie wrote:
> On Sun, Oct 13, 2019 at 2:58 PM Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > On Fri, 2019-09-27 at 10:59 +1000, Dave Airlie via Libclc-dev wrote:
> > > 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
>
> Thanks,
> Dave.
>
--
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20191028/31ab4510/attachment.sig>
More information about the Libclc-dev
mailing list