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

Add workaround for printing 128-bit types in gtest for windows #708

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

stanleytsang-amd
Copy link
Collaborator

See ROCm/hipCUB#462.

Testing to see if this fixes the CI build failure. Unable to reproduce locally.

@stanleytsang-amd stanleytsang-amd marked this pull request as ready for review March 28, 2025 22:16
@stanleytsang-amd stanleytsang-amd marked this pull request as draft March 29, 2025 01:58
@Snektron
Copy link
Contributor

I'm kind of confused why this workaround is suddenly required now. We were previously able to fully enable 128-bit integers in Windows and we observe no failures in our Windows CI, see b57e163. If you are seeing failures on your side, then perhaps we are both using different versions of the Windows SDK or MSVC compiler? It would be nice to know what those are so that we can use the same configuration on our CI.

@stanleytsang-amd
Copy link
Collaborator Author

stanleytsang-amd commented Mar 31, 2025

I'm kind of confused why this workaround is suddenly required now. We were previously able to fully enable 128-bit integers in Windows and we observe no failures in our Windows CI, see b57e163. If you are seeing failures on your side, then perhaps we are both using different versions of the Windows SDK or MSVC compiler? It would be nice to know what those are so that we can use the same configuration on our CI.

I'm not entirely sure either, we did not see the problem initially with our CI but it has now come up in our CI and also in staging, and also a compiler dev reported it as well. With the compiler dev's machine, I was able to resolve it by #706 , but that does not seem to work elsewhere. The reason I made this PR is because my local Windows machine does not exhibit this issue and I have limited ability to modify/update it at the moment, so I've been clumsily working via this PR.

Clearly it must be due to a difference in the HIP SDK build or the compiler version, as you stated. I will try to get that info. Unfortunately due to my limited understanding of the test assertion framework for rocPRIM, I've been unable to fix the problem - I'm clearly missing certain test cases/logic in the assertion code. @Snektron @Naraenda until I can nail down the exact HIP SDK build/compiler version, can you do a quick review and see what I'm missing? test_block_radix_sort fails to compile at the minimum. For a certainty it is related to 128-bit integers as my latest commit to disable them entirely in the unit testing causes the build to succeed (although it seemed to have hung at device_select...)

@Naraenda
Copy link
Member

Can you send us the build error (or entire log)? Either here or via e-mail. We can try narrowing it down on our side.

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.

3 participants