[PATCH] D77589: libclc: Add Mesa/SPIR-V/DXIL target

[Jan Vesely via Phabricator via llvm-commits]

jvesely added inline comments.


================
Comment at: libclc/CMakeLists.txt:95
+	list( APPEND LIBCLC_TARGETS_ALL spirv-mesa3d-dxil )
+endif()
+
----------------
automagic build targets are not nice, because they can fail silently.
I don't mind having dxil in ALL, but an error check if the user selected dxil and llvm-spirv tools were not found would be useful.


================
Comment at: libclc/CMakeLists.txt:250
 		# Some targets don't have a specific GPU to target
-		if( ${d} STREQUAL "none" )
+		if( ${ARCH} STREQUAL spirv )
 			set( mcpu )
----------------
This path is unnecessarily duplicating a lot of setup form `none` path.
Moving the spirv special case in a separate condition block below and only setting the extra flags would be a lot cleaner with fewer duplicated lines.


================
Comment at: libclc/CMakeLists.txt:254
+			set( t "spir--" )
+			set( dflags "-DCLC_SPIRV" )
+			set( oflags -O0 -finline-hint-functions )
----------------
`"-DCLC_${arch}" ` can be added globally for all targets.


================
Comment at: libclc/CMakeLists.txt:256
+			set( oflags -O0 -finline-hint-functions )
+			set( ooflags --structurizecfg )
+			if( ${OS} STREQUAL dxil )
----------------
`build_flags` and `opt_flags` would be more readable.


================
Comment at: libclc/CMakeLists.txt:286
 		target_compile_options( builtins.link.${arch_suffix} PRIVATE  -target
-			${t} ${mcpu} -fno-builtin )
+			${t} ${mcpu} -fno-builtin ${dflags} ${oflags} )
 		set_target_properties( builtins.link.${arch_suffix} PROPERTIES
----------------
`${dflags}` should be in `target_compile_definitions` rather than `target_compile_options`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77589/new/

https://reviews.llvm.org/D77589