CUDA: Add CUDA 11.8 / Hopper support and required fixes by gmarkall · Pull Request #8579 · numba/numba

gmarkall · 2022-11-04T12:29:54Z

See individual commit messages for full descriptions. The required fixes consist of:

Correcting the logic for determining supported compute capabilities, which was completely broken on main.
Using the correct NVVM IR version in all tests.

The logic for determining the supported compute capabilities was completely wrong, and needed updating in a complicated way for each new toolkit. This replaces the incorrect logic with a simpler mechanism: - We have a list of compute capabilities that we're aware of. - We have the minimum and maximum supported CC for each toolkit. - We generate the list of supported CCs as the range of known CCs within the min/max for the current toolkit. For unknown toolkits, all we can do is assume that it supports all the toolkits that we know about that aren't deprecated upstream (i.e. those from 5.3 onwards). This should be much easier to update - for each new toolkit, we just add a new entry in the mapping from toolkit versions to min/max supported CC versions. If we drop support for a toolkit, we can just delete it from the mapping.

The NVVM IR version for IR given to NVVM should always match the version that it supports - some versions of NVVM can reject IR where the version doesn't match. When the version isn't specified, it is implicitly 1.0, which is why this commit mostly consists of additions that add the IR version to modules.

gmarkall · 2022-11-04T12:30:00Z

gpuci run tests

stuartarchibald

Thanks for the patch. This largely looks good, just a few queries to resolve inline. Thanks again!

stuartarchibald · 2022-11-07T10:17:18Z

+    (9, 0)
+)
+
+# Maps CTK version -> (min supported cc, max supported cc)


Are the bounds inclusive/exclusive, should this be clarified?

Now clarified.

stuartarchibald · 2022-11-07T10:19:48Z

 def get_supported_ccs():
+    # Attempt to return cached list


If there's an in-process cache need, should something like functools.cache be used opposed to module globals?

That would be way better than the current bodge!

Actually it doesn't even need to be cached, this just makes no sense. The supported CCs is a property of the NVVM version, so I'm going to just make supported_ccs a property of the NVVM instance, because that just makes more sense anyway (and implicitly "caches" the values too because it's a singleton).

I think there could be an argument for the logic in get_supported_ccs() and its callees to be part of the NVVM class, but I think that functionality will also be common with NVPTX so I don't want to hoist the code into NVVM for now.

stuartarchibald · 2022-11-07T10:21:58Z

+    if cudart_version < (11, 0):
        _supported_cc = ()
+        ctk_ver = f"{cudart_version[0]}.{cudart_version[1]}"
+        unsupported_ver = (f"CUDA Toolkit {ctk_ver} is unsupported by Numba - "
+                           "11.0 is the minimum required version.")


Should the minimum version be specified as a constant somewhere (maybe a module global, or perhaps even all this encoding of supported CCs etc ought to go into it's own module or YAML file?) and then code be updated in terms of that constant?

I think instead of hardcoding the minimum CTK required, it can be derived from the dict of supported CCs, then it doesn't need to be stored separately anywhere. Additionally, I'm going to use NUMBA_CUDA_DEFAULT_PTX_CC for the default minimum for unrecognised toolkits instead of hardcoding (5, 3) above. This will be more robust in that a constant doesn't need updating in two places, and if there's some future toolkit that doesn't even support 5.3, the user can workaround it by setting the environment variable NUMBA_CUDA_DEFAULT_PTX_CC=6.1 (or whatever) to match the minimum for their toolkit.

The CC support matrix for NVPTX will differ, so I think keeping this encoding of supported CCs in nvvm.py is logical, and keeps complexity to a minimum instead of adding additional logic / dependencies.

stuartarchibald · 2022-11-07T10:24:38Z

+            versions = NVVM().get_ir_version()
+            metadata = metadata_nvvm70 % versions


Perhaps update to use .format() or an f-string?

I decided against that because it makes the IR less legible due to the presence of { and }.

stuartarchibald · 2022-11-07T10:49:29Z

        m = ir.Module("test_nvvm_from_llvm")
        m.triple = 'nvptx64-nvidia-cuda'
+        nvvm.add_ir_version(m)


Is this 3 line "setup" for a CUDA-target-LLVM-module sufficiently common to make it worth creating a utility function? The main concern here is that the m.triple specification is likely always necessary whereas the nvvm.add_ir_version(m) appears not to be? I suppose the underlying question is "what is the need driving the addition of the add_ir_version()" call?

This does happen for new modules created by the JITCUDACodegen, but I didn't use that here because it's part of the compiler / "cudapy", rather than the driver, which is under test in these cases.

It makes more logical sense to keep them on the NVVM instance as a property instead.

The minimum toolkit version can be derived from the support matrix, and the minimum CC for unrecognised toolkits can be derived from `config.CUDA_DEFAULT_PTX_CC`, which can also be overridden by the user if necessary - for example, if a user has a toolkit that in future has dropped even support for 5.3.

gmarkall · 2022-11-14T16:25:40Z

gpuci run tests

gmarkall · 2022-11-14T16:25:58Z

@stuartarchibald Many thanks for the review - I've addressed / responded to all comments.

stuartarchibald

@gmarkall Thanks for the fixes! I agree with the comments you left in response to review, thanks for answering/addressing them.

stuartarchibald · 2022-11-15T08:57:57Z

Buildfarm ID: numba_smoketest_cuda_yaml_172.

gmarkall · 2022-11-22T14:23:08Z

Any news on the buildfarm?

esc · 2022-12-01T09:34:49Z

@gmarkall @stuartarchibald apologies again for the delay here. Some hardware on the buildfram had to be replaced, hence the delay here. I have scheduled a fresh run for this PR at:

Build numba_smoketest_cuda_yaml_176 has started

esc · 2022-12-03T12:51:20Z

@gmarkall @stuartarchibald apologies again for the delay here. Some hardware on the buildfram had to be replaced, hence the delay here. I have scheduled a fresh run for this PR at:

Build numba_smoketest_cuda_yaml_176 has started

With #8636 merged to main, this was finally all green. 🍏

gmarkall added 2 commits November 4, 2022 12:19

gmarkall added 2 - In Progress CUDA CUDA related issue/PR Effort - medium Medium size effort needed 3 - Ready for Review and removed 2 - In Progress labels Nov 4, 2022

stuartarchibald reviewed Nov 7, 2022

View reviewed changes

stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Nov 7, 2022

stuartarchibald self-assigned this Nov 7, 2022

stuartarchibald added this to the Numba 0.57 RC milestone Nov 7, 2022

stuartarchibald added the highpriority label Nov 11, 2022

gmarkall added 3 commits November 14, 2022 16:10

NVVM: Don't cache supported CCs at global scope

e3e7d82

It makes more logical sense to keep them on the NVVM instance as a property instead.

Clarify that supported CC bounds are inclusive

c4e4081

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 14, 2022

stuartarchibald approved these changes Nov 14, 2022

View reviewed changes

stuartarchibald added 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 and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 14, 2022

esc removed the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Dec 3, 2022

esc 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 4 - Waiting on CI Review etc done, waiting for CI to finish labels Dec 3, 2022

esc merged commit 0bc5357 into numba:main Dec 3, 2022

		versions = NVVM().get_ir_version()
		metadata = metadata_nvvm70 % versions

Conversation

gmarkall commented Nov 4, 2022

Uh oh!

gmarkall commented Nov 4, 2022

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarkall commented Nov 14, 2022

Uh oh!

gmarkall commented Nov 14, 2022

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

stuartarchibald commented Nov 15, 2022

Uh oh!

gmarkall commented Nov 22, 2022

Uh oh!

esc commented Dec 1, 2022

Uh oh!

esc commented Dec 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants