Skip to content

Fix #7041: Add charseq registry to CUDA target - #7057

Merged
sklam merged 16 commits into
numba:masterfrom
gmarkall:grm-issue-7041
Nov 19, 2021
Merged

sklam merged 16 commits into
numba:masterfrom
gmarkall:grm-issue-7041

Conversation

@gmarkall

Copy link
Copy Markdown
Member

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.

The additional test is based on the reproducer in numba#7041.
@gmarkall gmarkall added 2 - In Progress CUDA CUDA related issue/PR Effort - short Short size effort needed labels May 26, 2021
@gmarkall

Copy link
Copy Markdown
Member Author

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 stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch, one minor thing to resolve else looks good.

Comment thread numba/cuda/tests/cudapy/test_const_string.py
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels May 26, 2021
@stuartarchibald stuartarchibald added this to the Numba 0.54 RC milestone May 26, 2021
Based on PR numba#7057 feedback

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@gmarkall

Copy link
Copy Markdown
Member Author

@stuartarchibald Many thanks for the review - suggestion applied.

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels May 26, 2021

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch!

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels May 26, 2021
@stuartarchibald

Copy link
Copy Markdown
Contributor

Buildfarm ID: numba_smoketest_cuda_yaml_71.

@stuartarchibald

Copy link
Copy Markdown
Contributor

Buildfarm ID: numba_smoketest_cuda_yaml_71.

Failed, nvvm segfault for test numba.cuda.tests.cudapy.test_const_string.py::test_assign_const_string, there's no obvious pattern to the failures in the test matrix.

@gmarkall

gmarkall commented Aug 9, 2021 •

Copy link
Copy Markdown
Member Author

Failed, nvvm segfault for test numba.cuda.tests.cudapy.test_const_string.py::test_assign_const_string, there's no obvious pattern to the failures in the test matrix.

@stuartarchibald Is it possible that CUDA toolkits < 11.2 failed and CUDA toolkits >= 11.2 passed?

@stuartarchibald

Copy link
Copy Markdown
Contributor

Failed, nvvm segfault for test numba.cuda.tests.cudapy.test_const_string.py::test_assign_const_string, there's no obvious pattern to the failures in the test matrix.

@stuartarchibald Is it possible that CUDA toolkits < 11.2 failed and CUDA toolkits >= 11.2 passed?

Output was:

Linux:
Passed: (py39, np1.20, cuda 11.2)
Failed: (py39, np1.20, cuda 10.0)
Failed: (py39, np1.20, cuda 9.2)

Passed: (py38, np1.17, cuda 11.2)
Failed: (py38, np1.17, cuda 10.0)
Passed: (py38, np1.17, cuda 9.2)

On windows all the combinations above failed except (py38, np1.17, cuda 11.2)

@gmarkall

Copy link
Copy Markdown
Member Author

Thanks for the details - I'm struggling to replicate the failure on Windows with (py39, np1.20, cuda 11.2) - my environment is:

(numbapy39np120) C:\work\numbadev\numba [grm-issue-7041 ≡]> conda list
# packages in environment at C:\work\miniconda3\envs\numbapy39np120:
#
# Name                    Version                   Build  Channel
backcall                  0.2.0              pyhd3eb1b0_0
blas                      1.0                         mkl
ca-certificates           2021.7.5             haa95532_1
certifi                   2021.5.30        py39haa95532_0
cffi                      1.14.6           py39h2bbff1b_0
colorama                  0.4.4              pyhd3eb1b0_0
cudatoolkit               11.2.2               h933977f_8    conda-forge
decorator                 5.0.9              pyhd3eb1b0_0
intel-openmp              2021.3.0          haa95532_3372
ipython                   7.26.0           py39hd4e2768_0
ipython_genutils          0.2.0              pyhd3eb1b0_1
jedi                      0.18.0           py39haa95532_1
llvmdev                   11.1.0                        2    numba
llvmlite                  0.38.0.dev0+14.g09df344           dev_0    <develop>
matplotlib-inline         0.1.2              pyhd3eb1b0_2
mkl                       2021.3.0           haa95532_524
mkl-service               2.4.0            py39h2bbff1b_0
mkl_fft                   1.3.0            py39h277e83a_2
mkl_random                1.2.2            py39hf11a4ad_0
numba                     0.54.0.dev0+636.gb52c31e4d           dev_0    <develop>
numpy                     1.20.3           py39ha4e8547_0
numpy-base                1.20.3           py39hc2deb75_0
openssl                   1.1.1k               h2bbff1b_0
parso                     0.8.2              pyhd3eb1b0_0
pickleshare               0.7.5           pyhd3eb1b0_1003
pip                       21.2.2           py39haa95532_0
prompt-toolkit            3.0.17             pyh06a4308_0
pycparser                 2.20                       py_2
pygments                  2.9.0              pyhd3eb1b0_0
python                    3.9.6                h6244533_0
setuptools                52.0.0           py39haa95532_0
six                       1.16.0             pyhd3eb1b0_0
sqlite                    3.36.0               h2bbff1b_0
traitlets                 5.0.5              pyhd3eb1b0_0
tzdata                    2021a                h52ac0ba_0
vc                        14.2                 h21ff451_1
vs2015_runtime            14.27.29016          h5e58377_2
wcwidth                   0.2.5                      py_0
wheel                     0.36.2             pyhd3eb1b0_0
wincertstore              0.2              py39h2bbff1b_0

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 (py38, np1.17, cuda 9.2) instance on Linux. If that proved to be the case, my plan was to only support charseqs and anything that builds on it when using CUDA 11.2 onwards, and strongly recommend the use of 11.2 with Numba from this point onwards (given that also lineinfo / debugging support and other things are also falling behind with toolkits < 11.2).

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 18, 2021
@gmarkall

Copy link
Copy Markdown
Member Author

@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.

@stuartarchibald

Copy link
Copy Markdown
Contributor

@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?

Comment thread numba/cuda/tests/cudapy/test_const_string.py
Comment thread numba/cuda/compiler.py
@gmarkall
gmarkall requested review from esc and sklam as code owners November 19, 2021 10:49
@gmarkall

Copy link
Copy Markdown
Member Author

gpuci run tests

Comment thread numba/cuda/tests/cudapy/test_const_string.py
Comment thread numba/cuda/tests/cudapy/test_const_string.py
Comment thread numba/cuda/tests/cudapy/test_const_string.py Outdated
@gmarkall

Copy link
Copy Markdown
Member Author

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
@gmarkall

Copy link
Copy Markdown
Member Author

gpuci run tests

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@stuartarchibald stuartarchibald added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm Effort - long Long size effort needed 4 - Waiting on CI Review etc done, waiting for CI to finish and removed BuildFarm Passed For PRs that have been through the buildfarm and passed 4 - Waiting on reviewer Waiting for reviewer to respond to author Effort - short Short size effort needed labels Nov 19, 2021
@stuartarchibald

Copy link
Copy Markdown
Contributor

Buildfarm ID: numba_smoketest_cuda_yaml_103.

@stuartarchibald stuartarchibald added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish labels Nov 19, 2021
@sklam
sklam merged commit 502cbd1 into numba:master Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed CUDA CUDA related issue/PR Effort - long Long size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants