[SYCL][E2E] Add logic to react to `REQUIRED`/`UNSUPPORTED` in `build-only` #16725

ayylol · 2025-01-21T21:08:38Z

Adds logic for evaluating REQUIRES/UNSUPPORTED statements on build-only mode by "ignoring" features in these statements that do not affect the compilation (Everything other than OS, triple, and SDK/libraries).

More precisely, the ability to ignore features is implemented by extending the boolean expressions to use a third boolean state - ignore. If a particular sub-expression includes an ignore, and if its result could be changed by setting that ignore to either true or false, then the result of that sub-expression is set to ignore. For example ignore || true = true, but ignore || false = ignore, this is because in the first example there would be no way to set the ignore such that the result is anything other than true, while in the second example the result is dependent on the "actual" value of the ignore.

If the resulting value of a REQUIRES predicate is ignore we interpret that as true (The requirements were met), on the other hand for UNSUPPORTED predicates we would interpret ignore as false instead (The unsupported criteria was not met).

The triples that can be used for a given test are then selected by evaluating the REQUIRES/UNSUPPORTED with the set of features + the target feature corresponding to each triple (mirroring the way we select devices), while ignoring all features that do not affect compilation.

Similarly to how XFAIL is handled when using multiple devices if we are compiling for multiple triples, and a single triple is marked as XFAIL, then it is treated as unsupported instead.

The available target triples in build-only mode is determined through the use of the sycl_triples lit param (i.e., --param sycl_triples=spir;amd). This is currently set to only spir, as the changes in test markup included in this pr do not take into account building for nvptx and amdgcn triples. In run-only and full mode the available triples are determined via the available devices.

ayylol · 2025-01-21T21:12:01Z

This pr is the logic from #16637, but only with support for spir64 triple, that way 205 file changes do not need to reviewed at once. Changes to the test markup to support amd and nvidia will be made in future prs.

sycl/test-e2e/TriStateExpr.py

aelovikov-intel · 2025-01-21T21:21:10Z

sycl/test-e2e/TriStateExpr.py

+        self.assertTrue(
+            E2EExpr.evaluate("!rt_feature", {"rt_feature"}, True, True)
+        )
+        self.assertTrue(
+            E2EExpr.evaluate("!!rt_feature", {"rt_feature"}, True, True)
+        )


IMO, there are far too many tests with {"rt_feature"} when run time features are to be ignored. We don't expect the set of "set" features to contain them, so why are we testing that scenario mostly?

I can understand having a couple of tests for completeness/to document behavior, but having too many is counterproductive and might confuse the reader about the purpose of this class.

Instead of deleting these, I just made the available features list empty. Does that work? otherwise I can delete them.

My intention with these was to show that ignore values propagate up the expression.

sycl/test-e2e/format.py

sarnex

quick initial review, will look more indepth once andrei's feedback is addressed

sycl/test-e2e/TriStateExpr.py

sarnex · 2025-01-21T21:43:54Z

sycl/test-e2e/format.py

+            if self.getMissingRequiredFeaturesFromList(features, test.requires, build_only_mode=True):
+                continue
+            if self.getMatchedFromList(features, test.unsupported, build_only_mode=True):


these functions are called get but are being used as has it seems, should we rename?

hasMissingRequiredFeatures and hasUnsupportedFeatures? or what were you thinking. One thing to note that brought some confusion in the past is the fact that these dont report the specific features that were missing to pass the expression, rather it returns the clauses that did not pass.

Also should note that getMissingRequiredFeaturesFromList is named after a function of the same name in the lit implementation. As for getMatchedFromList that one doesnt have an exact corresponding function, but there is getUnsupportedFeatures which is similar, but does not have an argument to pass in the features available.

do we ever use the return value's contents or is it just empty or not? if its only empty or not i think we should rename to has..., otherwise current is fine

One small caveat, the getMatchedFromList function is also used for determining XFAILs, so im not entirely sure hasUnsupportedFeatures would be entirely accurate.

This False implies that we're processing UNSUPPORTED, we need to modify the name of this function to indicate that now. Does lit.Test have some name that we can copy?

@aelovikov-intel tagging you here, because I think these comments are asking for the same thing, going to mark the other as resolved and we can continue discussion here

IMO, keeping it close to lit.Test or whatever the names came from is more important than renaming just because we don't use the result.

One small caveat, the getMatchedFromList function is also used for determining XFAILs, so im not entirely sure hasUnsupportedFeatures would be entirely accurate.

In this case, I'd avoid hardcoding False in the body and pass it as an additional parameter. Yes, it's kind of useless, but it would make interface more consistent.

yeah im not super familiar with the implementation of lit so if the function names are standard then its fine

sycl/test-e2e/format.py

sycl/test-e2e/lit.cfg.py

sycl/test-e2e/TriStateExpr.py

sycl/test-e2e/format.py

Maetveis · 2025-01-22T08:42:48Z

sycl/test-e2e/lit.cfg.py

+    target = config.backend_to_target[be]
+    features.add(target)
+    config.sycl_triples.add(target)


Backend is the underlying runtime that sycl is implemented on, right? Isn't it possible to have the same backend support multiple targets?. For example using OpenCL on AMD or NVIDIA (theoretically of course since practically they would have to support the extensions sycl requires).

I think if backend_to_target would be backend_default_target then it leaves the design open to add an override via some option like --param backend_targets_<be>=<target1>,<target2>.
Or maybe this is what architectures is for?

I think if backend_to_target would be backend_default_target then it leaves the design open to add an override via some option like --param backend_targets_<be>=<target1>,<target2>.
Or maybe this is what architectures is for?

Yeah, something like that could be possible to do by overriding/changing the dictionary used for the mappings. Although I do still prefer the x_to_y naming scheme for these mapping dictionaries, since it groups them together with consistent naming, and I don't think it necessarily stops us from redefining them in the future through parameters like you suggest.

Maetveis · 2025-01-22T08:47:00Z

sycl/test-e2e/lit.cfg.py

+config.target_to_triple = {
+    "target-spir": "spir64",
+    "target-nvidia":"nvptx64-nvidia-cuda",
+    "target-amd":"amdgcn-amd-amdhsa",
+    "target-native_cpu":"native_cpu",
+}
+config.triple_to_target = {v:k for k,v in config.target_to_triple.items()}


What's the difference between a "target" and the "triple" it corresponds to, given there's a 1:1 mapping between them? Is target just a short name for the triple? Is it an alias to allow changing the target triples without updating all the tests that refer to it?

Short name was part of the motivation (at least for me). Also, Nick mentioned that there is some work to have two different spir triples (spir64 and spirv, I think).

aelovikov-intel

Some minor naming suggestion, otherwise looks great.

aelovikov-intel · 2025-01-23T19:06:21Z

sycl/test-e2e/Regression/multiple-targets.cpp

@@ -2,8 +2,7 @@
 // It tests if the target triples can be specified with any order.
 // The test is repeated for per_kernel device code splitting.
 //
-// REQUIRES: cuda || hip || native_cpu
-// REQUIRES: build-and-run-mode
+// REQUIRES: target-nvidia || target-amd || target-native_cpu


Suggested change

// REQUIRES: target-nvidia || target-amd || target-native_cpu

// REQUIRES: (target-nvidia || target-amd || target-native_cpu) && target-spir

aelovikov-intel · 2025-01-23T19:13:59Z

sycl/test-e2e/E2EExpr.py

+        "false",
+    }
+
+    def __init__(self, string, variables, build_only_mode, findal_unknown_value):


Suggested change

def __init__(self, string, variables, build_only_mode, findal_unknown_value):

def __init__(self, string, variables, build_only_mode, final_unknown_value):

here and everywhere below.

aelovikov-intel · 2025-01-23T19:26:46Z

sycl/test-e2e/E2EExpr.py

+
+
+class TestE2EExpr(unittest.TestCase):
+    def test_basic(self):


I'd like to see this:

BuildOnly = True BuildAndRun = False RequiresDirective = True UnsupportedOrXFAILDirective = False self.assertTrue(E2EExpr.evaluate(<expr>, <features>, BuildOnly, RequiresDirective)

And now that I spelled it out, it can be even better (C++-like syntax, names to be improved):

auto BuildOnlyRequires = [](auto expr, auto features) { ... }; ... self.assertTrue(BuildOnlyRequires("linux", {"linux", "rt_feature"})) self.assertFalse(BuildOnlyUnsupported("rt_feature", {}))

Also, since we want these name improvements in the tests, we might as well want to propagate them all the way to the interfaces somehow.

aelovikov-intel · 2025-01-23T19:32:06Z

sycl/test-e2e/E2EExpr.py

+        )
+        self.assertTrue(
+            E2EExpr.evaluate(
+                "rt_feature", {"rt_feature"}, True, final_unknown_value=True


should we assert/raise an error instead?

aelovikov-intel · 2025-01-23T19:37:06Z

sycl/test-e2e/E2EExpr.py

+        )
+        self.assertTrue(
+            E2EExpr.evaluate(
+                "linux && !(windows || rt_feature)",


I have hard time mapping that to a possible REQURIES directive. I think changing "windows" to something non-OS might help, but even then I'm not sure what would be the best example. Same for the next one.

aelovikov-intel · 2025-01-23T19:38:23Z

sycl/test-e2e/E2EExpr.py

+        )
+        self.assertFalse(
+            E2EExpr.evaluate(
+                "linux && (windows && rt_feature)",


change "windows" to something that can make sense in real world. Same in the next one.

sarnex

no high level flags from me, will leave in-depth review to andrei/others who are more familiar with lit's internals

Add method to set triple + react to REQUIRES in build-only

123b4ac

ayylol requested review from sarnex, Maetveis, uditagarwal97 and aelovikov-intel January 21, 2025 21:08

ayylol requested review from a team as code owners January 21, 2025 21:08

ayylol temporarily deployed to WindowsCILock January 21, 2025 21:09 — with GitHub Actions Inactive

aelovikov-intel reviewed Jan 21, 2025

View reviewed changes

sycl/test-e2e/TriStateExpr.py Outdated Show resolved Hide resolved

aelovikov-intel reviewed Jan 21, 2025

View reviewed changes

sycl/test-e2e/format.py Outdated Show resolved Hide resolved

ayylol marked this pull request as draft January 21, 2025 21:34

aelovikov-intel reviewed Jan 21, 2025

View reviewed changes

sycl/test-e2e/format.py Outdated Show resolved Hide resolved

aelovikov-intel reviewed Jan 21, 2025

View reviewed changes

sycl/test-e2e/format.py Outdated Show resolved Hide resolved

sarnex reviewed Jan 21, 2025

View reviewed changes

ayylol temporarily deployed to WindowsCILock January 21, 2025 22:21 — with GitHub Actions Inactive

Maetveis reviewed Jan 22, 2025

View reviewed changes

sycl/test-e2e/TriStateExpr.py Outdated Show resolved Hide resolved

Maetveis reviewed Jan 22, 2025

View reviewed changes

sycl/test-e2e/format.py Outdated Show resolved Hide resolved

Maetveis reviewed Jan 22, 2025

View reviewed changes

ayylol added 3 commits January 22, 2025 07:09

Address comments related to new expressions

cdd3aa1

Rename triples to build_target where appropriate

0be2b74

Address 2 more comments

5a2b31f

ayylol temporarily deployed to WindowsCILock January 22, 2025 16:22 — with GitHub Actions Inactive

ayylol temporarily deployed to WindowsCILock January 22, 2025 17:36 — with GitHub Actions Inactive

ayylol added 2 commits January 22, 2025 11:31

Re-add REQUIRES: build-and-run-mode for tests failing on run-only

9a5f6c8

Add final_unknown_value as defaulted param in getMatchedFromList

d95ce2f

ayylol temporarily deployed to WindowsCILock January 22, 2025 20:06 — with GitHub Actions Inactive

ayylol temporarily deployed to WindowsCILock January 22, 2025 21:21 — with GitHub Actions Inactive

Add test to run E2EExpr unit tests, and move unit tests to separate file

a917988

ayylol had a problem deploying to WindowsCILock January 23, 2025 15:39 — with GitHub Actions Failure

Simplify code by using build_targets set throughout execute function

184d9d9

ayylol had a problem deploying to WindowsCILock January 23, 2025 16:13 — with GitHub Actions Error

Add requires linux on E2EExpr tests

d1f8409

ayylol had a problem deploying to WindowsCILock January 23, 2025 16:19 — with GitHub Actions Error

Move E2EExpr unit tests back into E2EExpr.py

cff4341

ayylol had a problem deploying to WindowsCILock January 23, 2025 17:06 — with GitHub Actions Error

Format changes

51c46b4

ayylol temporarily deployed to WindowsCILock January 23, 2025 18:47 — with GitHub Actions Inactive

aelovikov-intel reviewed Jan 23, 2025

View reviewed changes

ayylol temporarily deployed to WindowsCILock January 23, 2025 20:26 — with GitHub Actions Inactive

sarnex approved these changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][E2E] Add logic to react to `REQUIRED`/`UNSUPPORTED` in `build-only` #16725

[SYCL][E2E] Add logic to react to `REQUIRED`/`UNSUPPORTED` in `build-only` #16725

ayylol commented Jan 21, 2025

ayylol commented Jan 21, 2025

aelovikov-intel Jan 21, 2025

ayylol Jan 22, 2025

sarnex left a comment

sarnex Jan 21, 2025

ayylol Jan 22, 2025

sarnex Jan 22, 2025

ayylol Jan 22, 2025

aelovikov-intel Jan 22, 2025

aelovikov-intel Jan 22, 2025

sarnex Jan 23, 2025

Maetveis Jan 22, 2025 •

edited

Loading

ayylol Jan 22, 2025

Maetveis Jan 22, 2025

aelovikov-intel Jan 22, 2025

aelovikov-intel left a comment

aelovikov-intel Jan 23, 2025

aelovikov-intel Jan 23, 2025

aelovikov-intel Jan 23, 2025

aelovikov-intel Jan 23, 2025

aelovikov-intel Jan 23, 2025

aelovikov-intel Jan 23, 2025

sarnex left a comment

	// REQUIRES: target-nvidia \|\| target-amd \|\| target-native_cpu
	// REQUIRES: (target-nvidia \|\| target-amd \|\| target-native_cpu) && target-spir

	def __init__(self, string, variables, build_only_mode, findal_unknown_value):
	def __init__(self, string, variables, build_only_mode, final_unknown_value):

[SYCL][E2E] Add logic to react to REQUIRED/UNSUPPORTED in build-only #16725

Are you sure you want to change the base?

[SYCL][E2E] Add logic to react to REQUIRED/UNSUPPORTED in build-only #16725

Conversation

ayylol commented Jan 21, 2025

ayylol commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maetveis Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelovikov-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex left a comment

Choose a reason for hiding this comment

[SYCL][E2E] Add logic to react to `REQUIRED`/`UNSUPPORTED` in `build-only` #16725

[SYCL][E2E] Add logic to react to `REQUIRED`/`UNSUPPORTED` in `build-only` #16725

Maetveis Jan 22, 2025 •

edited

Loading