-
Notifications
You must be signed in to change notification settings - Fork 165
Thrust Overflow Fix #699
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
Thrust Overflow Fix #699
Conversation
…s to use long long to avoid overflow at >30 qubits. Fixes #698.
|
@JPRichings Fix branch for your testing pleasure! |
|
Single AMD GPU on ARCHER2 ✅: and okay one failure on 4 GPUs, but I believe that's entirely unrelated to this change... I'll try a 31/32 qubit QFT too. |
|
31-qubit QFT works and 32-qubit doesn't work on our AMD GPUs, as expected. I also checked and everything works fine with 32 qubits on 4 GPUs 👍 |
|
Eep good catch! Thrust literal mischief had already bitten me! To be extremely defensive, one could replace each QuEST/quest/src/core/bitwise.hpp Line 31 in 00ddd93
This would protect against future silent Thrust type issues if qindex was ever changed. And perhaps to evidence why that's a good idea, the QINDEX_ONE macro above is actually wrong since it treats qindex as unsigned, aha! You could correct that to 1LL in this patch too if you fancied :^)
Probably also worth then replacing that *It feels a little ill-fitting to define Let me know if you agree but don't have a sec, in which case I can add the changes to this PR! |
|
PS: I'll patch that "rightapplyCompMatr" error. It's because of this line... QuEST/tests/unit/operations.cpp Lines 1043 to 1047 in 00ddd93
which always uses a statevector to test the "targeted amps fit in node" validation, though the rightapply*() functions cannot accept statevectors, instead only density matrices. Because the "was given a density matrix" validation happens before "targeted amps fit in node" validation, the latter intended triggered error was beaten out by the earlier unintended one.
|
The operation validation tests previously always uses a statevector to test the "targeted amps fit in node" validation, though the rightapply*() functions cannot accept statevectors, instead only density matrices. Because the "was given a density matrix" validation happens before "targeted amps fit in node" validation, the latter intended triggered error was beaten out by the earlier unintended one. Now, we are careful to pass a density matrix Qureg to the validation of "targeted amps fit in node" when triggered by a function which 'right-applies' (and is ergo only compatible with density matrices)
|
My changes are passing (caution: testing only the affected functions) with nvcc v12.8 🙏 Happy to squash and co-commit as you fancy! |
|
Hi Tyson, Thanks for this! I like the principle of basing this on the Having looked at bitwise.hpp I don't see anything that should be an issue, but we should keep it in mind if we get any weird bugs 😆 I'll retest this branch on our GPU systems and then merge, all being well! |
|
Okay so there is still a rightapplyCompMatr test failure, but it is still unrelated to the GPU implementation -- it can be reproduced with any distributed build. I've spun that out into a new issue (#700) as all the other tests pass on the AMD GPUs, and importantly I can run the 31/32 qubit jobs no problem. As long as @JPRichings tests on the CUDA platforms pass I think we can merge this safely 😄 |
|
Updated branch passed tests on grace-hopper: QuEST execution environment: Testing configuration: Tested Qureg deployments: Randomness seeded to: 3270576438All tests passed (51882 assertions in 269 test cases) |
|
Excellent, thanks all. I'll merge. |
gpu_thrust.cuh: modified initial thrust counting iterator declarations to use long long to avoid overflow at >30 qubits. Fixes #698.