Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC][SYCL][E2E] Use REQUIRES/UNSUPPORTED in build-only mode #16528

Draft
wants to merge 4 commits into
base: sycl
Choose a base branch
from

Conversation

ayylol
Copy link
Contributor

@ayylol ayylol commented Jan 6, 2025

Currently the build-only mode does not take into account the features to decide if a test should be built on the system/for a particular triple. Instead explicit markup is used to mark these tests as unsupported.

A complication that occurs when trying to take into account features in build-only is that we do not have the devices that the tests are being built for, and thus we do not have the device specific features associated with them. Usually the device-specific features will not affect compilation, however this is not always the case, as certain features may implicitly exclude a particular triple. For example consider the feature accelerator. If we see a REQUIRES: accelerator this test would have never been built CUDA, or AMD triples, as those only support GPU devices. On the other hand however a UNSUPPORTED: accelerator statement would not exclude the spir triple, as that triple could be run on other non-accelerator devices.

Due to this we likely need to alter the way we use REQUIRES/UNSUPPORTED statements in order to support this. I'm looking for comments on what would be acceptable changes to our usage of those directives such that we can handle these statements in build-only mode while not adding too large of a burden on test writers.

This pr contains a potential solution which works by trying to construct a set of features to pass the REQUIRES/UNSUPPORTED statements. For REQUIRES statements we begin with the set of device-agnostic features that we already have from the environment, and add all the device-specific features listed in the statement that could possibly appear for the triple. For example, When considering a statement like REQUIRES: accelerator, the accelerator feature would be added when building for the spir64 triple, since it may or may not be present, however it would not be added for the nvptx-nvidia-cuda triple since we know for certain it could never be present for that triple. We consider UNSUPPORTED statements in a similar manner, however for the device-specific features here we only add them to our list of features if we know for certain a feature will always be present for a given triple. In this case if we see UNSUPPORTED: gpu we would not add the gpu feature for the spir64 triple, however we would add it for the nvptx-nvidia-cuda triple.

Because this method works off the assumption that we want all features listed in REQUIRES and none of the features listed in UNSUPPORTED this solution cannot support negations in either of these statements (i.e., REQUIRES: !gpu). The changes in this pr handle these situations by emitting a warning when a negation is found, and ignoring the clause including it when in build-only mode. This limitation only applies to 6 tests currently, and a majority of these negations can be removed by exchanging REQUIRES statements with UNSUPPORTED and vice versa. Currently the only problematic cases are the ones listed in test/e2e_test_requirements/no-negations-in-tests.cpp which contains statements in the form of UNSUPPORTED: x && !y

@ayylol
Copy link
Contributor Author

ayylol commented Jan 6, 2025

Some other ideas considered:

  • One possible direction would be to add extra directives like BUILD-REQUIRES. If a specific requires statements affects whether a test can be built or not for the environment/triple we could use this directive instead. It is unclear to me however how we would handle the device-specific features that affect compilation in this case (Like accelerator, as described above). Furthermore if a test does not compile it surely will not run either, so how the run-only mode reacts to these statements would also need to be considered.

  • Another suggested idea was to instead of creating a set of features to try to pass the REQUIRES/UNSUPPORTED statements, evaluating the expressions to see if any of the triples could possibly pass the REQUIRES/UNSUPPORTED statements. This would get around the negation limitation but would require a lot of code to implement the custom evaluation logic, which seems excessive to be able to support 6 test cases.

@ayylol ayylol marked this pull request as ready for review January 6, 2025 17:54
@ayylol ayylol requested review from a team as code owners January 6, 2025 17:54
@ayylol ayylol marked this pull request as draft January 6, 2025 18:24
"opencl-aot" :FeatureInfo(device_agnostic=True),
"ocloc" :FeatureInfo(device_agnostic=True),
"opencl_icd" :FeatureInfo(device_agnostic=True),
"cl_options" :FeatureInfo(device_agnostic=True),
Copy link
Contributor

@sarnex sarnex Jan 7, 2025

Choose a reason for hiding this comment

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

imo we have a relatively few number of tests that fail because of the triple or device stuff. i think supporting environment stuff like the OS is important, i think its fine to force those few number of tests require the build and run mode

having a hardcoded list of keywords and having different support for logical expressions than normal LIT kind of worries me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants