[SYCL] Add device config file consistency test #16369
Conversation
| @@ -532,6 +532,11 @@ if("hip" IN_LIST SYCL_ENABLE_BACKENDS) | |||
| list(APPEND SYCL_TOOLCHAIN_DEPLOY_COMPONENTS ur_adapter_hip) | |||
| endif() | |||
|
|
|||
| if(INSTALL_DEVICE_CONFIG_FILE) | |||
| add_dependencies(sycl-toolchain DeviceConfigFile) | |||
There was a problem hiding this comment.
can you give some background on why we need to install this? thx
There was a problem hiding this comment.
I'm still testing things, so I might change some things, but I want to support the new device_config_file_consistency test. It uses the DeviceConfigFile.hpp, and that file includes DeviceConfigFile.inc, which is generated by tablegen. On CI from what I understand the e2e tests are invoked by using the packed install files from the build step and only runs CMake on the sycl/test-e2e subfolder. So since we probably don't want to build tablegen and invoke other LLVM cmake files when running the e2e tests, I install the DeviceConfigFile.inc it in the build step to pass it to the e2e tests. Also note that this test must be an e2e test as it queries the device it is running the test on, so it can't be moved to sycl/test.
There was a problem hiding this comment.
sorry so what tools/files are required to generate that hpp file? Is it just llvm-tablegen and DeviceConfigFile.inc?
There was a problem hiding this comment.
Yea just tablegen and DeviceConfigFile.td are needed for the hpp file
There was a problem hiding this comment.
Also just for some more background I was aiming so that DeviceConfigFile.hpp is not installed by default because outside of testing, this file is not needed for a SYCL distribution, it is only used in the compiler.
| defvar CudaUSMAspects = [ | ||
| AspectUsm_device_allocations, AspectUsm_host_allocations, | ||
| AspectUsm_shared_allocations, AspectUsm_atomic_host_allocations, | ||
| AspectUsm_atomic_shared_allocations | ||
| ]; | ||
|
|
||
| defvar CudaMinAspects = !listconcat(CudaUSMAspects, [AspectGpu, AspectFp64, AspectOnline_compiler, AspectOnline_linker, | ||
| AspectQueue_profiling, AspectExt_intel_pci_address, AspectExt_intel_memory_bus_width, |
There was a problem hiding this comment.
From CI:
# | Checking consistency of aspects for device nvidia_gpu_sm_86...
# | error: nvidia_gpu_sm_86 does not support aspect usm_system_allocations but it is declared in the device config file
# | error: nvidia_gpu_sm_86 does not support aspect usm_atomic_host_allocations but it is declared in the device config file
# | error: nvidia_gpu_sm_86 does not support aspect ext_intel_max_mem_bandwidth but it is declared in the device config file
# | note: the device nvidia_gpu_sm_86 supports aspect ext_intel_legacy_image but it is not declared in the device config file
| AspectExt_intel_pci_address, AspectExt_intel_device_id, | ||
| AspectExt_intel_memory_clock_rate, AspectExt_intel_memory_bus_width, AspectExt_intel_free_memory]; | ||
|
|
||
| defvar HipUSMAspects = [ | ||
| AspectUsm_device_allocations, AspectUsm_host_allocations, | ||
| AspectUsm_shared_allocations, AspectUsm_atomic_host_allocations, | ||
| AspectUsm_atomic_shared_allocations | ||
| ]; | ||
|
|
There was a problem hiding this comment.
From CI:
# | Checking consistency of aspects for device amd_gpu_gfx1030...
# | error: amd_gpu_gfx1030 does not support aspect ext_intel_max_mem_bandwidth but it is declared in the device config file
# | error: amd_gpu_gfx1030 does not support aspect usm_system_allocations but it is declared in the device config file
There was a problem hiding this comment.
Ideally, we should also have a test which ensures consistency of values we can pass into -fsycl-targets and device config file and device_architecture extension, but that can be added as a follow-up. What's proposed here is already a good starting point to spot inconsistencies.
I'm a little concerned about us using std::string_view instead of llvm::StringRef, but at the same time I don't see anything which would prohibit us from using it. Moreover, an RFC to transition LLVM to std::string_view exists as well: https://discourse.llvm.org/t/migrating-llvm-stringref-to-std-string-view/82785
No description provided.