Skip to content
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

[SPIRV][DX] Share one test between backends #65975

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Conversation

Keenuts
Copy link
Contributor

@Keenuts Keenuts commented Sep 11, 2023

One big issue with DirectXShaderCompiler was test coverage: DXIL and SPIR-V backends had their own tests. When a bug was found in one, the other wasn't always checked. This lead to unequal support of HLSL for both backends. We'd like to avoid those issues here, hence the test-sharing.

By default, all the tests in this folder are marked as requiring DirectX. But as SPIR-V support grows, each test drop this requirement, and check the SPIR-V behavior.

I would have preferred to mark new tests as XFAIL for SPIR-V by default, so we could differentiate real unsupported tests (as SPIR-V has no equivalent), from newly added tests. But the way LIT is built, I don't think this is possible.

@Keenuts Keenuts requested a review from a team as a code owner September 11, 2023 16:13
@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 11, 2023

The buildkite failure seems to be related to the quoting of the - *_result.json. Seems unrelated to this change.

@llvm-beanz
Copy link
Collaborator

I don't think this is the right approach for test sharing. These tests all specifically test code in the DirectX backend, they aren't the kinds of tests that should be shared with the SPIR-V backend.

@bogner
Copy link
Contributor

bogner commented Sep 11, 2023

Would it make the most sense to have a place like test/Frontend/HLSL/ (to match lib/Frontend/HLSL - though there isn't such a directory yet)? This would provide a common place for HLSL specific constructs like numthreads and resources at least.

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 12, 2023

Would it make the most sense to have a place like test/Frontend/HLSL/ (to match lib/Frontend/HLSL - though there isn't such a directory yet)? This would provide a common place for HLSL specific constructs like numthreads and resources at least.

Yes, this makes sense. I don't know what the broader community will think of having frontend-specific tests here, but I do agree it would make sense.
Reverted the initial changes, and moved the shared test to the new folder.

Copy link
Contributor

@bogner bogner left a comment

Choose a reason for hiding this comment

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

This looks good to me. @llvm-beanz are you happy with this approach?

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

LGTM. At some point we'll want to expand the list of supported targets, but we can think about that later.

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 13, 2023

Thanks!
Added @michalpaszkowski on the SPIR-V side.

Copy link
Member

@michalpaszkowski michalpaszkowski left a comment

Choose a reason for hiding this comment

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

Thanks @Keenuts for working on this! LGTM!

One big issue with DirectXShaderCompiler was test coverage:
DXIL and SPIR-V backends had their own tests. When a bug was
found in one, the other wasn't always checked. This lead to
unequal support of HLSL for both backends. We'd like to avoid those
issues here, hence the test-sharing.

By default, all the tests in this folder are marked as requiring
DirectX. But as SPIR-V support grows, each test drop this requirement,
and check the SPIR-V behavior.

I would have preferred to mark new tests as XFAIL for SPIR-V by default,
so we could differentiate real unsupported tests (as SPIR-V has no
equivalent), from newly added tests. But the way LIT is built, I don't
think this is possible.

Signed-off-by: Nathan Gauër <brioche@google.com>
This reverts commit 24bd6a07bccab0ccb4f0d7f3370b4527dd3392d1.
rename Frontends to Frontend to match unittest folder
@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 21, 2023

rebased on main for merge

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 21, 2023

One failure on presubmit: python formatting. From the logs it's a bad argument, infra related (https://github.com/llvm/llvm-project/actions/runs/6259826929/job/16996615363?pr=65975).
Unrelated.

@Keenuts Keenuts merged commit 6bad175 into llvm:main Sep 21, 2023
2 of 3 checks passed
@Keenuts Keenuts deleted the test-sharing branch September 21, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants