-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Remove HIP Nvidia tests/CI support #16611
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
[SYCL] Remove HIP Nvidia tests/CI support #16611
Conversation
As noted in intel#14432 the configuration isn't supported/tested anymore and there were some PRs removing related `XFAIL`s (intel#16287, intel#16307, and possibly others). This PR goes a step further removes all the support for that configuration.
| if "amdgcn-amd-amdhsa" in triple: | ||
| llvm_config.with_system_environment("ROCM_PATH") | ||
| config.available_features.add("hip_amd") | ||
| config.available_features.add("hip") |
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.
Can we move this change to a separate commit or even PR?
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 don't see how, but maybe I don't fully understand the question.
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.
If I get it right, this change renames hip_amd lit feature into hip. The renaming makes sense to me, but it's not necessary for removing "HIP NVIDIA infrastructure support". The change contributes a significant part of changes to the diff, which will be easier to review separately.
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.
In practice that might be true, but in theory/semantically it's only possible after hip_nvidia removal. Also, the renaming part is only true for sycl/test while many of the changes are for sycl/test-e2e. There, it's not the renaming as hip LIT feature is pre-existing and means hip backend, which is true for both hip_nvidia and hip_amd. I can only change hip_amd->hip in tests' directives after/with the removal.
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've updated this PR to only modify our tests. I think that is, essentially, what you've asked for.
uditagarwal97
left a comment
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.
SYCL Changes LGTM.
jopperm
left a comment
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.
sycl-jit changes LGTM.
AlexeySachkov
left a comment
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.
Changes to Printf tests LGTM
As noted in #14432 the configuration isn't supported/tested anymore and there were some PRs removing related
XFAILs (#16287, #16307, and possibly others).This PR goes a step further and removes all the support for that configuration in our LIT tests.