[SYCL][E2E] Add test to check REQUIRES #16019

KornevNikita · 2024-11-07T18:50:20Z

This test checks the content of every "REQUIRES" to avoid typos and non-existing requirements.

This PR also updates some tests requirements found during test development.

This test checks the content of every "REQUIRES" to avoid typos and non-existing requirements. This PR also updates some tests requirements found during test development.

KornevNikita · 2024-11-07T18:52:57Z

The test will fail in pre-commit as we have REQUIRES: TEMPORARY_DISABLED. There is a PR to fix this - #15946

sycl/test-e2e/Basic/fpga_tests/fpga_aocx_win.cpp

sycl/test-e2e/Plugin/sycl-ls-gpu-default-level-zero.cpp

bader · 2024-11-07T19:46:19Z

sycl/test/e2e_test_requirements/check-correctness-of-requires.py

+
+
+def parse_requirements(input_data_path, sycl_include_dir_path):
+    available_features = {


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

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.

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.

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 ... }

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())

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.

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.

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

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.

It leads to dozens of these "detect" functions.

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?

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

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?

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.

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?

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.

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.

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.

bader · 2024-11-07T19:51:43Z

sycl/test/e2e_test_requirements/check-correctness-of-requires.cpp

+// 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


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

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

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

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.

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.

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

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.

@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

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

1. restore modified tests 2. use llvm-lit instead of grep 3. update a set of available features to match everything from lit.cfg.py

cperkinsintel · 2024-11-14T17:45:24Z

Can we also add documentation for what will be required to add a new supported value for the // REQUIRES and // XFAIL directives? Same for removal. All the places I will need to touch when I need to do this four months from now and have long forgotten about this PR.

That should be in lit.cfg.py , no?

KornevNikita · 2025-01-07T14:35:51Z

Accidentally created two commits instead of one: 473f187, 8160444

I moved the set of all features to a separate file, so it can be used in another python scripts. Now register function guarantees that new features can't miss this set. @bader @AlexeySachkov take a look please.

There is also an issue with --show-used-features doesn't show requests from local configs. I'll investigate it a bit later, after we agree on some general solution.

@cperkinsintel thanks for noting, I'll update the readme when the patch is finalized.

KornevNikita · 2025-01-30T11:27:28Z

@AlexeySachkov @bader could you please take a look?

@AlexeySachkov

I'll let @AlexeySachkov to decide on this one. Thanks for working on this!

aelovikov-intel · 2025-02-11T00:34:46Z

sycl/test-e2e/lit.cfg.py


 # test-mode: Set if tests should run normally or only build/run
 config.test_mode = lit_config.params.get("test-mode", "full")
 config.fallback_build_run_only = False
 if config.test_mode == "full":
-    config.available_features.add("run-mode")
-    config.available_features.add("build-and-run-mode")
+    register_feature(config, all_features, "run-mode", True)


Can we change the type of config.available_features so that its .add() would do the right thing automatically?

Hmm, the idea is good. But I'm not sure how to implement it.
Probably the available_features is defined here.
Not sure if we should change it. But may be we can create some available_features class here in lit.cfg.py and make its add function do it. So the code will look almost the same as before. WDYT?

Something like that, yes.

You can just add .add() method to whatever type is returned by get_all_sycl_lit_features(), rename that variable into available_features to make the code look closer to its current state.

Also: as long as our thing mimics Python array interface, we can just completely replace config.available_features with our own thing, i.e. config.available_features = our_custom_features_registry()

@aelovikov-intel I guess that's what you're looking for - 0976851

AlexeySachkov · 2025-02-12T10:00:55Z

sycl/test-e2e/lit.cfg.py


 # test-mode: Set if tests should run normally or only build/run
 config.test_mode = lit_config.params.get("test-mode", "full")
 config.fallback_build_run_only = False
 if config.test_mode == "full":
-    config.available_features.add("run-mode")
-    config.available_features.add("build-and-run-mode")
+    register_feature(config, all_features, "run-mode", True)


You can just add .add() method to whatever type is returned by get_all_sycl_lit_features(), rename that variable into available_features to make the code look closer to its current state.

Also: as long as our thing mimics Python array interface, we can just completely replace config.available_features with our own thing, i.e. config.available_features = our_custom_features_registry()

AlexeySachkov · 2025-02-12T10:15:36Z

sycl/test-e2e/lit.cfg.py

 elif config.test_mode == "run-only":
    lit_config.note("run-only test mode enabled, only executing tests")
-    config.available_features.add("run-mode")
+    register_feature(config, all_features, "run-mode", True)


One more thing here: let's say we dropped this line with register_feature("run-mode"). The feature itself is still listed in our known features - therefore it can be used to silently disable tests forever, right?

I suppose that extra changes should be made (and maintained) to make sure that register_feature is always called unconditionally.

Then at the end of lit.cfg.py we should compare list of registered features with list of known features.
If some known feature wasn't registered at all, then its a bug and we should highlight it. By "registered" here I mean passed to register_feature regardless of the last argument

So, I like how this PR looks compact with c448126, but now I don't know how to better implement what you talking about:D Any ideas?

so we don't to heavily modify existing code

aelovikov-intel · 2025-02-12T17:18:00Z

sycl/test-e2e/sycl_lit_allowed_features.py

+    "hip_amd",
+    "hip_nvidia",


I think we don't use these anymore.

aelovikov-intel · 2025-02-12T17:33:21Z

One other possible direction (that @ayylol and I haven't fully explored yet) is something like this:

https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/E2EExpr.py has support to evaluate the expression treating some features as "ignored" and setting the final value to "true"/"false" if the result depends on any such ignored feature.
It currently hardcodes build-only features

llvm/sycl/test-e2e/E2EExpr.py

Line 5 in fb888b8

build_specific_features = {

but we can hardcode run-only instead.
We use a docker image with all the SDK's installed in our CI task building E2E tests. I suppose we should strive to have all the tests buildable for at least one target. We can evaluate each test's REQUIRES "ignoring" runtime/per-device feature (possibly with extra processing for win-only). If a tests is still "UNSUPPORTED", then there is an error in the markup (either bad feature or incompatible expression of good ones).

This is very high-level and not known to work, but still worth bringing to this discussion.

[SYCL][E2E] Add test to check REQUIRES

239a975

This test checks the content of every "REQUIRES" to avoid typos and non-existing requirements. This PR also updates some tests requirements found during test development.

KornevNikita requested a review from AlexeySachkov November 7, 2024 18:50

KornevNikita requested review from a team as code owners November 7, 2024 18:50

KornevNikita requested a review from againull November 7, 2024 18:50

KornevNikita had a problem deploying to WindowsCILock November 7, 2024 18:51 — with GitHub Actions Failure

KornevNikita temporarily deployed to WindowsCILock November 7, 2024 19:20 — with GitHub Actions Inactive

bader previously requested changes Nov 7, 2024

View reviewed changes

Apply suggestions

d76ffef

1. restore modified tests 2. use llvm-lit instead of grep 3. update a set of available features to match everything from lit.cfg.py

KornevNikita had a problem deploying to WindowsCILock November 8, 2024 12:28 — with GitHub Actions Failure

KornevNikita temporarily deployed to WindowsCILock November 8, 2024 12:56 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/sycl' into check-requires

fc10f14

KornevNikita had a problem deploying to WindowsCILock November 12, 2024 15:25 — with GitHub Actions Error

allow feature

d8bdd59

KornevNikita had a problem deploying to WindowsCILock November 12, 2024 15:50 — with GitHub Actions Failure

KornevNikita temporarily deployed to WindowsCILock November 12, 2024 16:28 — with GitHub Actions Inactive

KornevNikita mentioned this pull request Jan 2, 2025

[SYCL][E2E] Align use of features in tests #16504

Open

KornevNikita added 3 commits January 3, 2025 03:12

Merge remote-tracking branch 'origin/sycl' into check-requires

d0f5fec

init

473f187

Update the solution

8160444

KornevNikita had a problem deploying to WindowsCILock January 7, 2025 14:29 — with GitHub Actions Failure

KornevNikita temporarily deployed to WindowsCILock January 7, 2025 14:57 — with GitHub Actions Inactive

ayylol mentioned this pull request Jan 16, 2025

[SYCL][E2E] Determine triples to build for on build-only (draft pr for testing all the triples) #16637

Closed

aelovikov-intel reviewed Feb 11, 2025

View reviewed changes

AlexeySachkov reviewed Feb 12, 2025

View reviewed changes

KornevNikita added 3 commits February 12, 2025 16:41

use custome type for available_features

0976851

so we don't to heavily modify existing code

Merge remote-tracking branch 'origin/sycl' into check-requires

fb1f123

align with sycl & format

c448126

KornevNikita had a problem deploying to WindowsCILock February 12, 2025 16:23 — with GitHub Actions Failure

KornevNikita had a problem deploying to WindowsCILock February 12, 2025 16:58 — with GitHub Actions Failure

aelovikov-intel reviewed Feb 12, 2025

View reviewed changes

KornevNikita added 2 commits February 12, 2025 19:08

update allowed features

2cd280c

move new class to fix pickle

513c796

KornevNikita had a problem deploying to WindowsCILock February 12, 2025 19:27 — with GitHub Actions Failure

KornevNikita had a problem deploying to WindowsCILock February 12, 2025 20:18 — with GitHub Actions Failure

KornevNikita mentioned this pull request Apr 15, 2025

[SYCL][E2E] Check for run-time features when in build-only #17988

Merged



		def parse_requirements(input_data_path, sycl_include_dir_path):
		available_features = {

[SYCL][E2E] Add test to check REQUIRES #16019

Are you sure you want to change the base?

[SYCL][E2E] Add test to check REQUIRES #16019

Uh oh!

Conversation

KornevNikita commented Nov 7, 2024

Uh oh!

KornevNikita commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KornevNikita Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Nov 14, 2024

Uh oh!

KornevNikita commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KornevNikita commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KornevNikita commented Nov 7, 2024 •

edited

Loading

AlexeySachkov Nov 7, 2024 •

edited

Loading

bader Nov 7, 2024 •

edited

Loading

KornevNikita Nov 8, 2024 •

edited

Loading

KornevNikita commented Jan 7, 2025 •

edited

Loading

aelovikov-intel Feb 11, 2025 •

edited

Loading