-
Notifications
You must be signed in to change notification settings - Fork 787
[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
Conversation
@aelovikov-intel @uditagarwal97 @sarnex Here is another approach for removing the Nick, to address some of the questions you had in the other draft prs:
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., 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?
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 |
There was a problem hiding this 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: |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
Hopefully i got all of them :)
build-only
via triple features in REQUIRES
statements build-only
(draft pr for testing all the triples)
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 onbuild-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 anignore
, and if its result could be changed by setting thatignore
to eithertrue
orfalse
, then the result of that sub-expression is set toignore
. For exampleignore || true = true
, butignore || 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 thantrue
, while in the second example the result is dependent on the "actual" value of theignore
.If the resulting value of a
REQUIRES
predicate isignore
we interpret that astrue
(The requirements were met), on the other hand forUNSUPPORTED
predicates we would interpretignore
asfalse
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 asXFAIL
, then it is treated as unsupported instead.The available triples in
build-only
mode is determined through the use of thesycl_triples
lit param (i.e.,--param sycl_triples=spir;amd
). By default this is set tospir;nvidia;amd
. Inrun-only
andfull
mode the available triples are determined via the available devices.