-
Notifications
You must be signed in to change notification settings - Fork 796
[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/UNSUPPORTEDstatements onbuild-onlymode 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 thatignoreto eithertrueorfalse, 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
REQUIRESpredicate isignorewe interpret that astrue(The requirements were met), on the other hand forUNSUPPORTEDpredicates we would interpretignoreasfalseinstead (The unsupported criteria was not met).The triples that can be used for a given test are then selected by evaluating the
REQUIRES/UNSUPPORTEDwith 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
XFAILis 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-onlymode is determined through the use of thesycl_tripleslit param (i.e.,--param sycl_triples=spir;amd). By default this is set tospir;nvidia;amd. Inrun-onlyandfullmode the available triples are determined via the available devices.