[SYCL][E2E] Implement support for REQUIRES in build-only mode #16716
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently if a test contains a malformed directive, the following
exception is raised along with the original parsing error:
```plaintext
Exception during script execution:
(original error)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "llvm/llvm/utils/lit/lit/worker.py", line 76, in _execute_test_handle_errors
result = test.config.test_format.execute(test, lit_config)
File "llvm/sycl/test-e2e/format.py", line 232, in execute
script = self.parseTestScript(test)
File "llvm/sycl/test-e2e/format.py", line 105, in parseTestScript
return lit.Test.Result(Test.UNRESOLVED, str(e))
NameError: name 'Test' is not defined
```
The test ends up as UNRESOLVED either way, but fixing it is easy and
improves the error message greatly.
These only apply when the test is going to be running i.e. only in `test_mode = "full"`` or `test_mode == "run-only"`. I want to use the unprefixed names in a follow up commit.
In general we cannot separate build only features from features that only have an effect during execution. Consider a `REQUIRES` expression containing a mix of runtime and build time features. If the expression evaluates to false in build-only we would have to solve a Satisfiability (SAT) problem to check if it only failed due to missing run-time features. The below design is motivated by this and the fact that current `REQUIRES`, `UNSUPPORTED` and `XFAIL` lines assume that both run-time (per-device) and build only features are available. Add a new directive `ENABLE_RUN_REQUIRES: true`, which, if present, changes the evaluation of `REQUIRES`, `UNSUPPORTED` and `XFAIL` directives to only consider device-agnostic features. This allows them to be correctly evaluated during `test-mode="build-only"` too. To express per-device (run-time) requirements the directives `RUN_REQUIRES`, `RUN_UNSUPPORTED` and `RUN_XFAIL` can be added. This commit does not yet change the documentation of the `test-mode` parameter in README.md, if this looks like a good direction I can do that too with help on the wording.
Maetveis
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Hey, I was working on something to address this in #16637, wonder if that may work for your use-case. |
Maetveis
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
I think it would, though I'm not sure the approach that is taken there is sound, I left comments. I feel like you're approaching the SAT solver territory, but I might be wrong. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In general we cannot separate
build only features from features that only have an effect during
execution. Consider a
REQUIRESexpression containing a mix of runtimeand build time features. If the expression evaluates to false in
build-only we would have to solve a Satisfiability (SAT) problem to
check if it only failed due to missing run-time features.
The below design is motivated by this and the fact that current
REQUIRES,UNSUPPORTEDandXFAILlines assume that both run-time(per-device) and build only features are available.
Add a new directive
ENABLE_RUN_REQUIRES: true, which, if present, changesthe evaluation of
REQUIRES,UNSUPPORTEDandXFAILdirectives toonly consider device-agnostic features. This allows them to be correctly
evaluated during
test-mode="build-only"too. To express per-device(run-time) requirements the directives
RUN_REQUIRES,RUN_UNSUPPORTEDand
RUN_XFAILcan be added.The config and directive takes a boolean value to allow gradual adoption
of the feature.
This commit does not yet change the documentation of the
test-modeparameter in README.md, if this looks like a good direction I can do
that too with help on the wording.
I also have not figured out how this should interact with #16306 yet, @ayylol input welcome.
I personally made these changes because I encountered this limitation during personal testing with build-only mode in Intel internal repositories, hopefully nobody else was actively working on this.