Skip to content

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

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

Merged
merged 7 commits into from
May 27, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions sycl/test-e2e/E2EExpr.py
Original file line number Diff line number Diff line change
@@ -36,9 +36,16 @@ class E2EExpr(BooleanExpression):
"vulkan",
"hip_options",
"cuda_options",
"host=None",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind explaining why we need to error if an unknown feature is true at build time? If there's an feature not in this list and it evaluates as true at build time doesn't that tell us it's a build-time feature and we don't need the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In build-only we ignore the run-time features because we expect them to always evaluate to false. So when a feature is not marked as build specific and it evaluates to true it means one of two things: It is a run-time feature which is incorrectly evaluating to true in build-only, or it is a build feature that was not added to the build features list, and thus we are incorrectly ignoring it. The latter is the more common case which I'm trying to make more visible. Since prior to these changes we would catch these issues only if the feature evaluated to true on the system, and was used in a REQUIRES/UNSUPPORTED.

The reason why we need the list, and cant just rely on the features that end up evaluating to true on build-only is because some of these features may evaluate to false on other systems/configurations, but we know that they should not be ignored. i.e., if we are missing the vulkan library we will fail building tests that require vulkan so this feature should not be ignored even if it evaluates to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for the clear explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

it means one of two things

Or, third option, this was mis-spelled and there is no such feature. Although maybe we catch that somewhere else now (there was a PR but I'm not sure if it was merged, @AlexeySachkov , @KornevNikita , @dm-vodopyanov might know?..)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this PR drowned in discussion (if you mean this one) ((maybe one day it will be finished)).

Copy link
Contributor

Choose a reason for hiding this comment

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

you linked to this pr, is that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

lol of course not, I mean this one - #16019

"target=None",
"shell",
"non-root-user",
"llvm-spirv",
"llvm-link",
"true",
"false",
"pdtracker",
"ze_debug",
}

def __init__(self, string, variables, build_only_mode, final_unknown_value):
@@ -66,14 +73,10 @@ def evaluate(string, variables, build_only_mode, final_unknown_value=True):
def parseMATCH(self):
token = self.token
BooleanExpression.parseMATCH(self)
if token not in self.build_specific_features and self.build_only_mode:
if token not in E2EExpr.build_specific_features and self.build_only_mode:
self.unknown = True
else:
self.unknown = False
if self.value and self.unknown:
raise ValueError(
'Runtime feature "' + token + '" evaluated to True in build-only'
)

def parseAND(self):
self.parseNOT()
@@ -113,6 +116,18 @@ def parseAll(self):
self.expect(BooleanExpression.END)
return self.final_unknown_value if self.unknown else self.value

@staticmethod
def check_build_features(variables):
rt_features = [x for x in variables if x not in E2EExpr.build_specific_features]
if rt_features:
raise ValueError(
"Runtime features: "
+ str(rt_features)
+ " evaluated to True in build-only\n"
+ "If this is a new build specific feature append it to:"
+ "`build_specific_features` in `sycl/test-e2e/E2EExpr.py`"
)


import unittest

@@ -165,11 +180,12 @@ def test_basic(self):
self.assertFalse(
UnsupportedBuildEval("linux && (vulkan && rt_feature)", {"linux"})
)
# runtime feature is present in build-only
# Check that no runtime features are present in build-only
with self.assertRaises(ValueError):
RequiresBuildEval("rt_feature", {"rt_feature"})
E2EExpr.check_build_features({"rt-feature"})
with self.assertRaises(ValueError):
UnsupportedBuildEval("rt_feature", {"rt_feature"})
E2EExpr.check_build_features({"build-only", "rt-feature"})
E2EExpr.check_build_features({"build-mode"})


if __name__ == "__main__":
6 changes: 6 additions & 0 deletions sycl/test-e2e/lit.cfg.py
Original file line number Diff line number Diff line change
@@ -1098,6 +1098,12 @@ def get_sycl_ls_verbose(sycl_device, env):
clangxx += config.cxx_flags
config.substitutions.append(("%clangxx", clangxx))

# Check that no runtime features are available when in build-only
from E2EExpr import E2EExpr

if config.test_mode == "build-only":
E2EExpr.check_build_features(config.available_features)

if lit_config.params.get("print_features", False):
lit_config.note(
"Global features: {}".format(" ".join(sorted(config.available_features)))