[SYCL] Add architecture info to sycl-ls --verbose #13976
Conversation
| auto arch = Device.get_info<syclex::info::device::architecture>(); | ||
| switch (arch) { | ||
| #define __ARCH(ARCH) \ | ||
| case syclex::architecture::ARCH: \ |
There was a problem hiding this comment.
I think we should refactor this into a .inc file that would be used both here and in device_info.hpp, so that updates would need to be made in a single place.
sycl/tools/sycl-ls/sycl-ls.cpp
Outdated
| @@ -68,6 +68,106 @@ std::string getDeviceTypeName(const device &Device) { | |||
| } | |||
| } | |||
|
|
|||
| namespace syclex = sycl::ext::oneapi::experimental; | |||
There was a problem hiding this comment.
This has to be at least local to the getArchName function, if not fully inlined.
| @@ -684,11 +688,17 @@ | |||
| sg_sizes = set(dev_sg_sizes[0]).intersection(*dev_sg_sizes) | |||
| lit_config.note("SG sizes for {}: {}".format(sycl_device, ", ".join(sg_sizes))) | |||
|
|
|||
| lit_config.note("Architectures for {}: {}".format(sycl_device, ", ".join(architectures))) | |||
| if len(architectures) != 1 or "unknown" in architectures: | |||
| architectures = set() | |||
There was a problem hiding this comment.
I think it would be safer to just raise an error and abandon testing.
There was a problem hiding this comment.
If we see more (or less) than one architecture returned, we should indeed abandon testing and report that as some fatal failure.
However, if we see unknown we should proceed, I think. This is important for our downstream where we launch those tests on new/unstable/unknown hardware and we don't really want to have a complete blocker like this in there. It is ok that some of the tests may be skipped because they require a certain architecture, but at least the rest of the testing will go through.
There was a problem hiding this comment.
Can we require an explicit option telling us that "unknown" is fine? What I want to avoid is this auto-detection silently failing and then not running tests that we should be running on a particular runner/HW and thinking we have good test coverage there due to not seeing any failures when in reality the tests are simply skipped.
There was a problem hiding this comment.
What I want to avoid is this auto-detection silently failing and then not running tests that we should be running on a particular runner/HW and thinking we have good test coverage there due to not seeing any failures when in reality the tests are simply skipped.
We could have a test for the detection which will fail indicating us that. And accepting unknown could be done as a downstream customization.
Anyways, the opt-in to allow unknown looks good to me as well. I think it is up to you, @aelovikov-intel, I'm not a code owner in this area.
sycl/tools/sycl-ls/sycl-ls.cpp
Outdated
| #undef __SYCL_ARCHITECTURE | ||
| #undef __SYCL_ARCHITECTURE_ALIAS | ||
| } | ||
| assert(false && "Unreachable!"); |
There was a problem hiding this comment.
We now have unknown architecture. I suppose that it should technically be a default: clause within the switch, but it is hard to achieve, so I suggest that we just return unknown here to avoid warnings that you see on Windows
#13976 adds support for using the `Architecture` output of `sycl-ls` when configuring `e2e` tests. This PR adds an opt-in option to allow configuring `e2e` tests when the architecture is `unknown`.
Previously we had to explicitly specify that certain HW is present to enable corresponding LIT features and make some tests run (or be skipped). Forgetting to set that argument could lead to missing coverage, or unrelated failures. intel#13976 introduced automatic detection of HW architecture to E2E tests. This patch is one in a series of transitioning our tests to use that new functionality. This patch transitions tests that use `gpu-intel-pvc` feature to use auto-generated `architecture-intel_gpu_pvc`. The patch does not completely removes `gpu-intel-pvc` usage in our LIT scripts, because some other features depend on it and I would like to do changes incrementally.
Previously we had to explicitly specify that certain HW is present to enable corresponding LIT features and make some tests run (or be skipped). Forgetting to set that argument could lead to missing coverage, or unrelated failures. #13976 introduced automatic detection of HW architecture to E2E tests. This patch is one in a series of transitioning our tests to use that new functionality. This patch transitions tests that use `gpu-intel-pvc` feature to use auto-generated `arch-intel_gpu_pvc`. The patch does not completely removes `gpu-intel-pvc` usage in our LIT scripts, because some other features depend on it and I would like to do changes incrementally.
This PR adds architecture info from the sycl_ext_oneapi_device_architecture to
sycl-ls --verbose. Additionally, the E2E lit config is updated to parse this info and add these as features that can be used in lit testing (e.g.// REQUIRES: architecture-intel_gpu_pvc).