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

CUDA: Fix linkage of device functions when compiling for debug #7162

Merged
merged 7 commits into from Jun 30, 2021

Conversation

gmarkall
Copy link
Member

@gmarkall gmarkall commented Jun 29, 2021

Fixes #7159.

@gmarkall gmarkall changed the title CUDA: Fix linkage of device functions when compiling for debug [WIP CUDA: Fix linkage of device functions when compiling for debug [WIP] Jun 29, 2021
@gmarkall gmarkall marked this pull request as draft June 29, 2021 11:37
@gmarkall
Copy link
Member Author

@esc @stuartarchibald Is this possible for a CUDA buildfarm run to see if it turns up any issues on any of the other platforms please?

@stuartarchibald
Copy link
Contributor

@esc @stuartarchibald Is this possible for a CUDA buildfarm run to see if it turns up any issues on any of the other platforms please?

Buildfarm ID: numba_smoketest_cuda_yaml_80.

@stuartarchibald
Copy link
Contributor

@esc @stuartarchibald Is this possible for a CUDA buildfarm run to see if it turns up any issues on any of the other platforms please?

Buildfarm ID: numba_smoketest_cuda_yaml_80.

Failed on CUDA < 11.2 with Error: Debugging support cannot be enabled when named metadata !llvm.dbg.cu is not defined.

@stuartarchibald stuartarchibald added this to the Numba 0.54 RC milestone Jun 29, 2021
@sklam
Copy link
Member

sklam commented Jun 29, 2021

I can get the reproducer in the original issue to work If I turn off the debug option when !llvm.dbg.cu is not in the LLVM IR like below:

diff --git a/numba/cuda/codegen.py b/numba/cuda/codegen.py
index 70dcb10d6..5cfdaf6f1 100644
--- a/numba/cuda/codegen.py
+++ b/numba/cuda/codegen.py
@@ -125,7 +125,7 @@ class CUDACodeLibrary(serialize.ReduceMixin, CodeLibrary):
         else:
             # Otherwise, we compile all modules with NVVM at once because this
             # results in better optimization than separate compilation.
-            ptxes = [nvvm.llvm_to_ptx(irs, **options)]
+            ptxes = [nvvm.llvm_to_ptx(ir, **options) for ir in irs]
 
         # Sometimes the result from NVVM contains trailing whitespace and
         # nulls, which we strip so that the assembly dump looks a little
diff --git a/numba/cuda/cudadrv/nvvm.py b/numba/cuda/cudadrv/nvvm.py
index eb631b703..963022c5c 100644
--- a/numba/cuda/cudadrv/nvvm.py
+++ b/numba/cuda/cudadrv/nvvm.py
@@ -706,6 +706,9 @@ def llvm_to_ptx(llvmir, **opts):
     for mod in llvmir:
         mod = llvm_replace(mod)
         cu.add_module(mod.encode('utf8'))
+
+        if "!llvm.dbg.cu" not in mod:
+            opts.pop("debug", None)
     cu.lazy_add_module(libdevice.get())
 
     return cu.compile(**opts)

however, the test will still fail with:

numba.cuda.cudadrv.driver.LinkerError: [999] Call to cuLinkComplete results in CUDA_ERROR_UNKNOWN
error   : Undefined reference to '_ZN5numba4cuda5tests6cudapy14test_debuginfo17TestCudaDebugInfo29_test_chained_device_function12$3clocals$3e11kernel$2422Eii' in '<cudapy-ptx>'

@gmarkall
Copy link
Member Author

@esc @sklam could this have another buildfarm run please?

@sklam
Copy link
Member

sklam commented Jun 30, 2021

BFID: numba_smoketest_cuda_yaml_81

@sklam sklam added the CUDA CUDA related issue/PR label Jun 30, 2021
@sklam
Copy link
Member

sklam commented Jun 30, 2021

BF passed!

@gmarkall gmarkall changed the title CUDA: Fix linkage of device functions when compiling for debug [WIP] CUDA: Fix linkage of device functions when compiling for debug Jun 30, 2021
@gmarkall gmarkall marked this pull request as ready for review June 30, 2021 15:08
@sklam sklam added 3 - Ready for Review BuildFarm Passed For PRs that have been through the buildfarm and passed and removed 2 - In Progress labels Jun 30, 2021
sklam
sklam previously approved these changes Jun 30, 2021
Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

The PR looks good to me.

This PR will disable debuginfo on NVVM version < 7.0 and it is acceptable because the debug support is not ideal in the old (2014) version. We shouldn't spend too much time fixing for the old toolchain given that the CUDA community is likely to be on the latest CTK with the newer NVVM, for which Numba generates better debuginfo now.

debug_callee = any(lib._nvvm_options.get('debug')
for lib in self.linking_libraries)
if options.get('debug') or debug_callee:
msg = "debuginfo is not generated for CUDA versions < 11.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

From OOB discussion, this message is going to be updated to include the function name so it's easy for users to find the function(s) causing this warning.

We now emit one warning per function with debug=True when compiling on
NVVM 3.4, with the warning message specifying the function name to help
the user see the source of the warning.
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, couple of minor comments else looks good.

numba/cuda/codegen.py Outdated Show resolved Hide resolved
numba/cuda/tests/cudapy/test_debuginfo.py Show resolved Hide resolved
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@gmarkall
Copy link
Member Author

@stuartarchibald Thanks for the review - I believe comments are addressed / answered now.

@gmarkall gmarkall added the 4 - Waiting on reviewer Waiting for reviewer to respond to author label Jun 30, 2021
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch and fixes.

@stuartarchibald
Copy link
Contributor

NOTE: I checked out the patch and manually tested the warnings etc in local code on a CTK=11.0. Also checked that the tests are fine under CTK=11.0 and CTK=11.2.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 30, 2021
@stuartarchibald
Copy link
Contributor

Buildfarm ID: numba_smoketest_cuda_yaml_82 ran successfully on 68c4e76.

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jun 30, 2021
@sklam sklam merged commit a29b40f into numba:master Jun 30, 2021
DrTodd13 pushed a commit to IntelLabs/numba that referenced this pull request Jul 22, 2021
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CUDA regression: Mixing debug kwargs between called functions results in linkage error
3 participants