Skip to content
3 changes: 1 addition & 2 deletions sycl/test-e2e/Basic/fpga_tests/fpga_aocx_win.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
//
//===----------------------------------------------------------------------===//

// REQUIRES: opencl-aot, accelerator
// REQUIRES: system-windows
// REQUIRES: opencl-aot, accelerator, windows

/// E2E test for AOCX creation/use/run for FPGA
// Produce an archive with device (AOCX) image. To avoid appending objects to
Expand Down
2 changes: 1 addition & 1 deletion sycl/test-e2e/Plugin/sycl-ls-gpu-default-level-zero.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// REQUIRES: gpu, level-zero
// REQUIRES: gpu, level_zero

// TODO: Remove unsetting SYCL_DEVICE_FILTER when feature is dropped
// RUN: env --unset=SYCL_DEVICE_FILTER --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out
Expand Down
2 changes: 1 addition & 1 deletion sycl/test-e2e/Regression/msvc_crt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// RUN: %{run} %t1.exe
// RUN: %{build} /MDd -o %t2.exe
// RUN: %{run} %t2.exe
// REQUIRES: system-windows, cl_options
// REQUIRES: windows, cl_options
//==-------------- msvc_crt.cpp - SYCL MSVC CRT test -----------------------==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// (because only SPIR-V supports specialization constants natively)

// FIXME: This set is never satisfied all at once in our infrastructure.
// REQUIRES: opencl, level-zero, cpu, gpu, opencl-aot, ocloc
// REQUIRES: opencl, level_zero, cpu, gpu, opencl-aot, ocloc

// RUN: %clangxx -fsycl -DJIT %s -o %t.out
// RUN: %{run} %t.out
Expand Down
10 changes: 10 additions & 0 deletions sycl/test/e2e_test_requirements/check-correctness-of-requires.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This test checks that all "REQUIRES" strings contain the right feature names
// If this test fails:
// 1. there is some typo/non-existing feature request in the
// modified test.
// 2. ...or, there is some new feature. In this case please update the set of
// features in check-correctness-of-requires.py
//
// RUN: grep -rI --include=*.cpp "REQUIRES: " %S/../../test-e2e | sed -E 's|.*/test-e2e/||' > %t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't use grep! It's error prone. We must leverage llvm-lit scripts to get the list of features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't use grep! It's error prone

Just out of curiosity, what sort of errors can we run into with grep?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can write a custom format, which would automatically read requirements for every test and compare them with reference. Something like I've done for checking that every header can be included standalone here.

I don't even think that we would need to redefine discovery step, we just need to redefine the test step: instead of running any commands from the file we would just check requirements and fail if we encountered anything unknown. The only thing which we will need is to somehow make lit discover tests in a different directory for us.

Copy link
Contributor

@bader bader Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't use grep! It's error prone

Just out of curiosity, what sort of errors can we run into with grep?

This script already has a bug. llvm-lit is configured to consider .c files as test files as well, but there might be other file extension for the tests in test-e2e directory.
Someone can use "REQUIRES: " in the manner that llvm-lit ignores (e.g. in the middle of the comments).

We rely on some logic of llvm-lit scripts, but we don't leverage the source code of these scripts at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can write a custom format, which would automatically read requirements for every test

This part is already done in the llvm-lit.

--show-used-features Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit

llvm-lit --show-used-features llvm/test/

aarch64-registered-target amdgpu-registered-target arm-registered-target asserts avr-registered-target bpf-registered-target can-execute csky-registered-target curl cxx-shared-library debug debug_frame default_triple diasdk disabled do-not-run-me ed. examples exegesis-can-execute-aarch64 exegesis-can-execute-mips exegesis-can-execute-x86_64 exegesis-can-measure-latency exegesis-can-measure-latency-lbr expensive_checks fma3 has_logf128 have_tf_aot have_tflite hexagon-registered-target host-byteorder-little-endian host={{.*-windows-msvc}} httplib intel-jitevents intel_feature_xe lanai-registered-target ld_emu_elf32ppc libcxx-used libxml2 lld llvm-64-bits llvm-driver llvm-dylib llvm_inliner_model_autogenerated llvm_raevict_model_autogenerated loongarch-registered-target mips-registered-target msan msp430-registered-target native non-root-user nvptx-registered-target object-emission plugins powerpc-registered-target reverse_iteration riscv-registered-target riscv64-registered-target shell sparc-registered-target spirv-tools static-libs system-aix system-darwin system-freebsd system-linux system-netbsd system-windows system-zos systemz-registered-target target-byteorder-little-endian target-x86_64 target=aarch64-pc-windows-{{.*}} target=aarch64{{.*}} target=amdgcn-{{.*}} target=arm{{.*}} target=avr{{.*}} target=hexagon-{{.*}} target=loongarch{{.*}} target=nvptx{{.*}} target=powerpc64-unknown-linux-gnu target=powerpc64{{.*}} target=powerpc{{.*}} target=r600{{.*}} target=riscv{{.*}} target=sparc{{.*}} target=target=powerpc64-unknown-linux-gnu target=x86_64-{{.*-ps[45]}} target=xcore{{.*}} target={{(aarch64|arm).*}} target={{(i686|i386).*}} target={{(mips|mipsel)-.*}} target={{.*-(cygwin|windows-gnu|windows-msvc)}} target={{.*-(cygwin|windows-msvc|windows-gnu)}} target={{.*-linux.*}} target={{.*-windows-(gnu|msvc)}} target={{.*-windows.*}} target={{.*windows.*}} target={{.*}} target={{.*}}-aix{{.*}} target={{.*}}-apple{{.*}} target={{.*}}-windows-gnu target={{.*}}-zos{{.*}} target={{i686.*windows.*}} thread_support to-be-fixed use_msan_with_origins uses_COFF vc-rev-enabled ve-registered-target vg_leak webassembly-registered-target x86-registered-target x86_64-apple x86_64-linux xar zlib zstd

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script already has a bug. llvm-lit is configured to consider .c files as test files as well, but there might be other file extension for the tests in test-e2e directory.

We have previously agreed with Nikita that treating .c files as E2E tests was a bug and it should be fixed. We don't have any .c tests and we can compile SYCL code (which requires C++17) in C mode. But in general I tend to agree, there is a request to introduce .sycl file extension, for example.

Copy link
Contributor Author

@KornevNikita KornevNikita Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bader thanks! llvm-lit sycl/test-e2e --show-used-features works good, I'll try it.
UPD the only issue - now we can't say which test exactly contains the wrong feature requirement. But I guess developers can figure it out themselves.
d76ffef

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note here, --show-used-features does not show features added to requires/unsupported/xfail from lit.local.cfg files.

// Using a python script as it's easier to work with sets there
// RUN: python3 %S/check-correctness-of-requires.py %t %sycl_include
116 changes: 116 additions & 0 deletions sycl/test/e2e_test_requirements/check-correctness-of-requires.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
# See check-correctness-of-requires.cpp

import re
import sys


# To parse .def files to get aspects and architectures
def parse_defines(path, macro, prefix):
features = set()
with open(path, "r") as file:
for line in file:
if line.startswith(macro):
feature = line.split("(")[1].split(",")[0].strip()
features.add(f"{prefix}-{feature}")
return features


def parse_requirements(input_data_path, sycl_include_dir_path):
available_features = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must import features from llvm/sycl/test-e2e/lit.cfg.py instead of building a manually maintained copy of this list.

Copy link
Contributor

@AlexeySachkov AlexeySachkov Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must import features from llvm/sycl/test-e2e/lit.cfg.py instead of building a manually maintained copy of this list.

Can we really do that in a reliable manner? Those features depend on the environment (OS, available tools, build configuration, enabled projects, etc.).

I.e. it is expected that REQUIRES may reference features which are not registered. But some of those could be known and therefore legal, whilst others could be a result of a typo and therefore should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must import features from llvm/sycl/test-e2e/lit.cfg.py instead of building a manually maintained copy of this list.

+1 to Alexey's question. lit.cfg.py generates the set of available features "online" according to the specific env. Do you know if it's possible to get the whole set regardless of env? I agree it's not the best idea to maintain this set manually, but I'm not sure if we have another options.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea, but it requires refactoring of llvm/sycl/test-e2e/lit.cfg.py.
We can have a dictionary of all features with callbacks like this:

features = {
'x': available, # 'available' function just returns true, i.e. make 'x' always available.
'y': is_y_available, # 'is_y_available' function returns true if feature 'y' is available.
'z': available, # 'available' function just returns true
...
}
  1. To get the list of all features that can be used by e2e tests we just need to get the keys (i.e. features.keys())
  2. To build the list of available features, we just go over all the items in the dictionary and use functions to decide which features are available.

The benefit of this implementation, you don't need to update your test when new feature is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there always available features?
I like that the suggested approach is more "centralized", but I guess it will take some time as it requires some further discussion. I'd prefer to merge this PR as is, since I don't think we get new features really often. Then we can proceed with a follow-up patch to move this set to lit.cfg.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to prototype this and have a few questions:

  1. Do we really need to proceed with such dictionary and not the set? Such dictionary requires the developer to not only add a key for new feature, but also a function to detect the availability of this feature.
  2. It leads to dozens of these "detect" functions.
  3. And it looks like then we can't auto generate aspect and architecture features (I mean as it's done now in this patch), as they may require some specific approach to detect them.

In total from my POV the idea of such dictionary looks too complicated. Looks like it requires a thorough refactoring now and a lot of work in future to support it, however, I don't see much benefit from this.

I suggest to keep it as set and move it to lif.cfg.py as function, so we can call it from this test. Something like:

def get_all_features:
    all_features {
        "cpu",
        "gpu",
        ...
    }
    all_features.update(generate_aspects())
    all_features.update(generate_archs())
    return all_features

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to prototype this and have a few questions:

  1. Do we really need to proceed with such dictionary and not the set? Such dictionary requires the developer to not only add a key for new feature, but also a function to detect the availability of this feature.

The main benefit of using dictionary is that it is a single source of all features. What I'd like to avoid is to build two separate lists of features: all possible features and all available features. The idea with dictionary allows us to build "all available features" list automatically. Current proposal requires developers to add features into two lists separately and it's easy to miss. I'm open to alternative ideas, which will guarantee that if developer adds new llvm-lit feature to the list of "available features", there is no way to skip the feature in the list of "all possible features".

Your test is kind of guarantees that to some extent i.e. it's supposed to fail if e2e tests will use some llvm-lit feature X, which is not in the list of available_features defined by sycl/test/e2e_test_requirements/check-correctness-of-requirements.py. Am I right?
 

  1. It leads to dozens of these "detect" functions.

Right. Is that a problem? We have all this logic already. It's just not structured as separate "detect" functions.

  1. And it looks like then we can't auto generate aspect and architecture features (I mean as it's done now in this patch), as they may require some specific approach to detect them.

Why not?

In total from my POV the idea of such dictionary looks too complicated. Looks like it requires a thorough refactoring now and a lot of work in future to support it, however, I don't see much benefit from this.

I agree that it requires refactoring now, but I hope you see the benefit for future changes, which I tried to explain above. If you have any better ideas, I'm all ears.

I suggest to keep it as set and move it to lif.cfg.py as function, so we can call it from this test. Something like:

def get_all_features:
    all_features {
        "cpu",
        "gpu",
        ...
    }
    all_features.update(generate_aspects())
    all_features.update(generate_archs())
    return all_features

What do you think?

How is this different from your current patch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current proposal requires developers to add features into two lists separately and it's easy to miss.

If a feature added as available, but left unused - it doesn't matter if we list it in possible features and it is likely a user error, which doesn't sound too serious, we can ignore that. For example, we have FileCheck registered as a feature, but I doubt that we use it anywhere.

which will guarantee that if developer adds new llvm-lit feature to the list of "available features", there is no way to skip the feature in the list of "all possible features".

If a feature added as available, adds it use, but isn't registered as "known", this new test will fail, notifying a PR author.

Therefore, I don't think that we can miss anything important when we adding features.

However, if a feature was removed from available features, but kept in "known" - we won't highlight any incorrect uses of it. From that point of view, the approach proposed by @bader is better, I think.
And we do remove features from time to time - the most often used example is renaming of aspects added by experimental extensions.

It leads to dozens of these "detect" functions.
Right. Is that a problem? We have all this logic already. It's just not structured as separate "detect" functions.

I think it worth exploring that path. lit.cfg.py is already quite lengthy and I wonder if we can structure it better. I don't think that we really need "detect" functions, but we probably need a helper functions to register features. It would accept two arguments: a feature itself and a boolean saying whether its available. Under the hood it will unconditionally register a feature as known, but conditionally register it as available.

if platform.system() == "Linux":
    config.available_features.add("linux")
    llvm_config.with_system_environment(["LD_LIBRARY_PATH", "LIBRARY_PATH", "CPATH"])
    llvm_config.with_environment(
        "LD_LIBRARY_PATH", config.sycl_libs_dir, append_path=True
    )

Would turn into:

if platform.system() == "Linux":
    register_feature(config, "linux", True)
    llvm_config.with_system_environment(["LD_LIBRARY_PATH", "LIBRARY_PATH", "CPATH"])
    llvm_config.with_environment(
        "LD_LIBRARY_PATH", config.sycl_libs_dir, append_path=True
    )

# For simpler cases:
register_feature(config, "my-new-os", platform.system() == "New OS")

And that doesn't look that bad. We have plenty of features which are detected by running an external process - there is plenty of duplicated code and I would love to see some refactoring there to have less repetitions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with registering function.

One very minor concern I have is using config across lit.cfg.py. I suppose we can delay updating the list of available features until all features are registered. Another approach is to create a wrapper object containing config and make register function a method of this function object. If it's not too much trouble it would be nice to avoid using config directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One very minor concern I have is using config across lit.cfg.py. I suppose we can delay updating the list of available features until all features are registered.

Strictly speaking, there could be nested lit.local.cfg files which register extra features and requirements for certain sub-folders. Therefore, at the very end of lit.cfg may not yet be a final place where all possible features are known.

Perhaps what we can do is to make our custom test format, which would first check the requirements of tests and then fallback to a regular lit runner.

# host OS:
"windows",
"linux",
# target device:
"cpu",
"gpu",
"accelerator",
# target backend:
"cuda",
"hip",
"opencl",
"level_zero",
"native_cpu",
"hip_amd",
# tools:
"sycl-ls",
"cm-compiler",
"aot_tool",
"ocloc",
"opencl-aot",
"llvm-spirv",
"llvm-link",
# dev-kits:
"level_zero_dev_kit",
"cuda_dev_kit",
# manually-set features (deprecated, no new tests should use these features)
"gpu-intel-gen11",
"gpu-intel-gen12",
"gpu-intel-dg1",
"gpu-intel-dg2",
"gpu-intel-pvc",
"gpu-intel-pvc-vg",
"gpu-amd-gfx90a",
# any-device-is-:
"any-device-is-cpu",
"any-device-is-gpu",
"any-device-is-accelerator",
"any-device-is-cuda",
"any-device-is-hip",
"any-device-is-opencl",
"any-device-is-level_zero",
# sg-sizes (should we allow any sg-X?)
"sg-8",
"sg-16",
"sg-32",
# miscellaneous:
"cl_options",
"opencl_icd",
"dump_ir",
"xptifw",
"has_ndebug",
"zstd",
"preview-breaking-changes-supported",
"vulkan",
# Note: aspects and architectures are gathered below
}

available_features.update(
parse_defines(
sycl_include_dir_path + "/sycl/info/aspects.def", "__SYCL_ASPECT", "aspect"
)
)
available_features.update(
parse_defines(
sycl_include_dir_path + "/sycl/info/aspects_deprecated.def",
"__SYCL_ASPECT",
"aspect",
)
)
available_features.update(
parse_defines(
sycl_include_dir_path + "/sycl/ext/oneapi/experimental/architectures.def",
"__SYCL_ARCHITECTURE",
"arch",
)
)

exit_code = 0
with open(input_data_path, "r") as file:
for line in file:
# get the content of "REQUIRES: "
requirements = re.compile(r"// REQUIRES: (.*)").search(line)
# Drop all symbols except feature names
requirements = re.split(r"&&|\|\||, |,|\(|\)|\s+", requirements.group(1))
# Filter out empty names
requirements = [req.strip() for req in requirements if req.strip()]

for feature in requirements:
# some names can start with "!" e.g. "!level_zero", drop "!"
feature = feature.lstrip("!")
if not feature in available_features:
exit_code = 1
print(line + "contains unsupported feature: " + feature)
sys.exit(exit_code)


parse_requirements(sys.argv[1], sys.argv[2])
2 changes: 1 addition & 1 deletion sycl/test/regression/sycl-include-gnu17.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// RUN: %clangxx -std=gnu++17 -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
// RUN: %t.out

// UNSUPPORTED: system-windows
// UNSUPPORTED: windows

#include <iostream>
#include <sycl/sycl.hpp>
Expand Down
Loading