[NFCI][SYCL] Pass SPIR-V extensions explicitly to jit compiler #17359
Conversation
Resolves intel#16597 Signed-off-by: Sidorov, Dmitry <[email protected]>
| // Keep this in sync with clang/lib/Driver/ToolChains/Clang.cpp | ||
| // TODO: consider introducing a config file that both clang and jit-compiler | ||
| // could use during options setting. | ||
| std::vector<SPIRV::ExtensionID> AllowedExtensions { |
There was a problem hiding this comment.
Can we keep the same order as in Clang.cpp?
I'm trying to check that the lists match, but I find it quite challenging with the order changed.
There was a problem hiding this comment.
To make my life easier I have copied all of the extension we have in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/include/LLVMSPIRVExtensions.inc and removed unused in Clang.cpp extensions. I have also not included AOT FPGA extensions and some other extensions, like SPV_INTEL_debug_module - which are being deprecated. Like this order is not preserved. Do you really what it to have the same order?
There was a problem hiding this comment.
We will have to constantly keep this in sync with Clang.cpp, which is hard to see if the order very different. For example, SPV_EXT_shader_atomic_float16_add appears last-but-one in Clang.cpp, but first here.
Would it help you to just copy the list from Clang.cpp and apply a bit of search-and-replace to get the right format?
Then, the maintenance task of keeping them in sync becomes easier, because they'll have the same order (ignoring FPGA).
There was a problem hiding this comment.
SPIRV::ExtensionID::SPV_EXT_shader_atomic_float_add, SPIRV::ExtensionID::SPV_EXT_shader_atomic_float_min_max,
SPIRV::ExtensionID::SPV_KHR_no_integer_wrap_decoration, SPIRV::ExtensionID::SPV_KHR_float_controls,
SPIRV::ExtensionID::SPV_KHR_expect_assume, SPIRV::ExtensionID::SPV_KHR_linkonce_odr,
SPIRV::ExtensionID::SPV_INTEL_subgroups, SPIRV::ExtensionID::SPV_INTEL_media_block_io,
SPIRV::ExtensionID::SPV_INTEL_device_side_avc_motion_estimation,
SPIRV::ExtensionID::SPV_INTEL_fpga_loop_controls, SPIRV::ExtensionID::SPV_INTEL_unstructured_loop_controls,
SPIRV::ExtensionID::SPV_INTEL_fpga_reg, SPIRV::ExtensionID::SPV_INTEL_blocking_pipes,
SPIRV::ExtensionID::SPV_INTEL_function_pointers, SPIRV::ExtensionID::SPV_INTEL_kernel_attributes,
SPIRV::ExtensionID::SPV_INTEL_io_pipes, SPIRV::ExtensionID::SPV_INTEL_inline_assembly,
SPIRV::ExtensionID::SPV_INTEL_arbitrary_precision_integers,
SPIRV::ExtensionID::SPV_INTEL_float_controls2, SPIRV::ExtensionID::SPV_INTEL_vector_compute,
SPIRV::ExtensionID::SPV_INTEL_fast_composite,
SPIRV::ExtensionID::SPV_INTEL_arbitrary_precision_fixed_point,
SPIRV::ExtensionID::SPV_INTEL_arbitrary_precision_floating_point,
SPIRV::ExtensionID::SPV_INTEL_variable_length_array, SPIRV::ExtensionID::SPV_INTEL_fp_fast_math_mode,
SPIRV::ExtensionID::SPV_INTEL_long_composites,
SPIRV::ExtensionID::SPV_INTEL_arithmetic_fence,
SPIRV::ExtensionID::SPV_INTEL_global_variable_decorations,
SPIRV::ExtensionID::SPV_INTEL_cache_controls,
SPIRV::ExtensionID::SPV_INTEL_fpga_buffer_location,
SPIRV::ExtensionID::SPV_INTEL_fpga_argument_interfaces,
SPIRV::ExtensionID::SPV_INTEL_fpga_invocation_pipelining_attributes,
SPIRV::ExtensionID::SPV_INTEL_fpga_latency_control,
SPIRV::ExtensionID::SPV_KHR_shader_clock,
SPIRV::ExtensionID::SPV_INTEL_bindless_images,
SPIRV::ExtensionID::SPV_INTEL_task_sequence,
SPIRV::ExtensionID::SPV_INTEL_bfloat16_conversion,
SPIRV::ExtensionID::SPV_INTEL_joint_matrix,
SPIRV::ExtensionID::SPV_INTEL_hw_thread_queries,
SPIRV::ExtensionID::SPV_KHR_uniform_group_instructions,
SPIRV::ExtensionID::SPV_INTEL_masked_gather_scatter,
SPIRV::ExtensionID::SPV_INTEL_tensor_float32_conversion,
SPIRV::ExtensionID::SPV_INTEL_optnone,
SPIRV::ExtensionID::SPV_KHR_non_semantic_info,
SPIRV::ExtensionID::SPV_KHR_cooperative_matrix,
SPIRV::ExtensionID::SPV_EXT_shader_atomic_float16_add,
SPIRV::ExtensionID::SPV_INTEL_fp_max_error
There was a problem hiding this comment.
We will have to constantly keep this in sync with Clang.cpp
That is why I left a comment to consider introducing a unified config for both clang and jit-compiler :)
There was a problem hiding this comment.
We've had this comment for multiple months now, so I think we should prepare for a non-trivial amount of time where this needs to be maintained manually.
I think investing a bit of time (e.g., using the list I posted above) now will save us maintenance burden in the next months.
We should probably also add a comment in Clang.cpp that the list is to be kept in sync with this location here, so that people enabling new extensions in Clang.cpp are aware they need to maintain this place.
Resolves #16597