Skip to content

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

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

Closed
wants to merge 17 commits into from

Conversation

ayylol
Copy link
Contributor

@ayylol ayylol commented Jan 14, 2025

NOTE: This is draft-pr is for testing only to see if we can build for all the triples with this approach.

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 triple as a feature (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 triples in build-only mode is determined through the use of the sycl_triples lit param (i.e., --param sycl_triples=spir;amd). By default this is set to spir;nvidia;amd. In run-only and full mode the available triples are determined via the available devices.

@ayylol
Copy link
Contributor Author

ayylol commented Jan 16, 2025

@aelovikov-intel @uditagarwal97 @sarnex

Here is another approach for removing the REQUIRES: build-and-run-mode markup, and building for multiple triples.

Nick, to address some of the questions you had in the other draft prs:

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

This method doesn't have the same limitation with supporting negations in lit expressions, because an ignored feature negated is still ignored just the same. The one case we might need to look out for that we have had in the past is where we mark a test as unsupported by marking it unsupported for a feature that is also required, i.e., REQUIRES: gpu and UNSUPPORTED: gpu, the feature would be ignored in both statements so it would run in build-only. Since those are always unsupported they should probably just be marked with UNSUPPORTED: * if they fail to build.

With regards to the hardcoded list of keywords. There was a suggestion to add a unified way to register the features that can be used in e2e tests in #16019, I think if that idea goes through it would make sense to add the info that a particular feature affects compilation could be done through the "register" function suggested. From what I've found there is only three types of features we need to worry about, OS, triples, and SDKs/libraries. I imagine OS, and triple features would rarely be added, SDKs/libraries maybe a little more often?

also i wonder if we can do anything smart to minimize the vars users need to set, like if we see UNSUPPORTED: hip_amd maybe we can automatically not compile for amdgcn-amd-amdhsa at the build step

What I've done in this pr is add the corresponding triple to the available devices depending on their backend. Doing this we don't need to duplicate markup, since a hip device would have the amdgcn-amd-amdhsa feature, and thus it would be marked as unsupported for UNSUPPORTED: amdgcn-amd-amdhsa. Furthermore, the separation of backend and triple gives us more expressiveness in what exactly we are saying is unsupported. if we use the backend (UNSUPPORTED: hip) we are saying that the test cannot execute on that backend, it may however be able to compile for it. On the other hand using the triple (UNSUPPORTED: amdgcn-amd-amdhsa), we are specifically saying that the test cannot compile for the triple, and by extension it wont be able to execute on devices with the corresponding backend either.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

high level looks good to me!

return supported_triples
# Treat XFAIL as UNSUPPORTED if the test is to be compiled for multiple
# triples.
if "*" in test.xfails:
Copy link
Contributor

Choose a reason for hiding this comment

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

kinda unrelated question, i know we do the same today for multiple target devices but does it seem like it has to be this way or do it seem like it can be fixed if we put effort in to (which shouldnt be done as part of this pr obv)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're closer to being able to fix it now, so worth another look. @uditagarwal97 and I had talked about that recently. IMO, we should finish these "split" activities first so that changes won't have too many conflicts (both source/git-level and logical ones).

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, that would be a great fix because so many bugs are missed/nonsense happens because of that behavior, but this is def more important :)

@ayylol ayylol changed the title [SYCL][E2E] Determine what triples to build for on build-only via triple features in REQUIRES statements [SYCL][E2E] Determine triples to build for on build-only (draft pr for testing all the triples) Jan 21, 2025
@ayylol ayylol closed this Jan 31, 2025
@ayylol ayylol deleted the tristate-expr branch January 31, 2025 14:36
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.

4 participants