Repository navigation
Fix #7041: Add charseq registry to CUDA target - #7057
Conversation
The additional test is based on the reproducer in numba#7041.
|
Unsure of what milestone to suggest - this should be a short PR to deal with, but I'm aware the queue for 0.54 is already insanely long. |
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch, one minor thing to resolve else looks good.
Based on PR numba#7057 feedback Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
|
@stuartarchibald Many thanks for the review - suggestion applied. |
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch!
|
Buildfarm ID: |
Failed, |
@stuartarchibald Is it possible that CUDA toolkits < 11.2 failed and CUDA toolkits >= 11.2 passed? |
Output was: Linux: Passed: (py38, np1.17, cuda 11.2) On windows all the combinations above failed except (py38, np1.17, cuda 11.2) |
|
Thanks for the details - I'm struggling to replicate the failure on Windows with Do you see any discrepancies between my setup and the failing Windows + CUDA 11.2 setup? If not, do you have any more details about the failure? (e.g. a .dmp file, some details from the event log at the time of the crash, any traceback presented, etc.)? My hypothesis was that this is an NVVM 3.4 vs NVVM 7.0 thing - i.e. that the failures were running into an NVVM 3.4 bug - because its a memory corruption, it could sometimes pass on older toolkit versions by luck, like the |
|
@stuartarchibald Thanks for the legalization suggestion. I've gone with it, but made it raise instead of warn, since character sequences dangerous on NVVM 3.4 and never worked before anyway. I also created #7579 to record the fact that this still doesn't enable character sequences in record on CUDA. I think this is ready for another look now. |
|
@gmarkall No problem. I agree with the raise instead of warn, given it is a dangerous/unpredictable route to go down. Should a note be added to the docs to this effect i.e. that with CTK 11.2+ the CUDA target now has charseq support? |
|
gpuci run tests |
Also add some relevant tests
|
gpuci run tests |
The string field (`d`) isn't actually read / written in any test here (probably because that would have generated some code that segfaulted NVVM 3.4) but is used to exercise alignment of arrays of the record. Changing the type here to a `uint8` should fulfil this purpose, but avoid triggering the legalization check with NVVM 3.4
|
gpuci run tests |
stuartarchibald
left a comment
There was a problem hiding this comment.
Many thanks for all your efforts on this patch, I think this should permit charseq support where it is supported by the nvvm compiler and prevent it otherwise.
|
Buildfarm ID: |
The additional test is based on the reproducer in #7041.
It seems that this should enable support for other string functions in CUDA that are related to this - tests for these may be added in a follow-up PR.