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

Implement array reshape for CUDA #47

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dlee992
Copy link
Contributor

@dlee992 dlee992 commented Aug 23, 2024

I ported numba/numba#8458 to this repo. The original PR is quite complete.

  • add a common util function for linking usage link_to_library_functions.
  • Newly added tests are passed locally.

Anything else need to be tackled?

Copy link

copy-pr-bot bot commented Aug 23, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@gmarkall
Copy link
Collaborator

/ok to test

@gmarkall gmarkall added the 3 - Ready for Review Ready for review by team label Aug 23, 2024
@gmarkall gmarkall changed the base branch from develop to main October 7, 2024 13:29
@gmarkall
Copy link
Collaborator

gmarkall commented Oct 7, 2024

I've just re-targeted this to main as that's where all future PRs should go now that main seems OK WRT Numba upstream.

@gmarkall gmarkall added this to the v0.0.19 milestone Oct 21, 2024
@gmarkall
Copy link
Collaborator

I merged main into this to test it on Windows; it's failed there due to a mismatching prototype:

======================================================================
ERROR: test_array_reshape (numba.cuda.tests.cudapy.test_cuda_array_interface.TestCudaArrayInterface.test_array_reshape)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 2881, in add_ptx
    driver.cuLinkAddData(self.handle, enums.CU_JIT_INPUT_PTX,
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 352, in safe_cuda_api_call
    self._check_ctypes_error(fname, retcode)
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 420, in _check_ctypes_error
    raise CudaAPIError(retcode, msg)
numba.cuda.cudadrv.driver.CudaAPIError: [300] Call to cuLinkAddData results in CUDA_ERROR_INVALID_SOURCE

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\tests\cudapy\test_cuda_array_interface.py", line 485, in test_array_reshape
    check(array_reshape, array_reshape1d, arr, (24,))
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\tests\cudapy\test_cuda_array_interface.py", line 464, in check
    kernel[1, 1](arr, shape, got)
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\dispatcher.py", line 598, in __call__
    return self.dispatcher.call(args, self.griddim, self.blockdim,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\dispatcher.py", line 740, in call
    kernel = _dispatcher.Dispatcher._cuda_call(self, *args)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\dispatcher.py", line 748, in _compile_for_args
    return self.compile(tuple(argtypes))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\dispatcher.py", line 992, in compile
    kernel.bind()
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\dispatcher.py", line 254, in bind
    self._codelibrary.get_cufunc()
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\codegen.py", line 226, in get_cufunc
    cubin = self.get_cubin(cc=device.compute_capability)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\codegen.py", line 203, in get_cubin
    linker.add_file_guess_ext(path)
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 2699, in add_file_guess_ext
    self.add_cu_file(path_or_code)
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 2684, in add_cu_file
    self.add_cu(cu, os.path.basename(path))
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 2675, in add_cu
    self.add_ptx(ptx.encode(), ptx_name)
  File "D:\numbadev\numba-cuda\numba_cuda\numba\cuda\cudadrv\driver.py", line 2884, in add_ptx
    raise LinkerError("%s\n%s" % (e, self.error_log))
numba.cuda.cudadrv.driver.LinkerError: [300] Call to cuLinkAddData results in CUDA_ERROR_INVALID_SOURCE
error   : Prototype doesn't match for 'numba_attempt_nocopy_reshape' in 'reshape_funcs.ptx', first defined in 'reshape_funcs.ptx'

However, I don't know if this is a Windows issue or an issue arising from the merge, or me resolving conflicts incorrectly. To try and work it out, I've pushed up the merged version to see what happens in CI here.

@gmarkall
Copy link
Collaborator

/ok to test

@gmarkall
Copy link
Collaborator

Looks like this has a Windows-related issue. I'll fix style so I don't leave this PR in a worse state than when I encountered it, and see if I can work out what's going wrong on Windows.

@gmarkall
Copy link
Collaborator

/ok to test

@gmarkall
Copy link
Collaborator

On Windows, the implementation of the function has the following signature:

.visible .func  (.param .b32 func_retval0) numba_attempt_nocopy_reshape(
        .param .b32 numba_attempt_nocopy_reshape_param_0,
        .param .b64 numba_attempt_nocopy_reshape_param_1,
        .param .b64 numba_attempt_nocopy_reshape_param_2,
        .param .b32 numba_attempt_nocopy_reshape_param_3,
        .param .b64 numba_attempt_nocopy_reshape_param_4,
        .param .b64 numba_attempt_nocopy_reshape_param_5,
        .param .b32 numba_attempt_nocopy_reshape_param_6,
        .param .b32 numba_attempt_nocopy_reshape_param_7
)

However, the Numba-generated code declares it as:

.extern .func  (.param .b32 func_retval0) numba_attempt_nocopy_reshape
(
        .param .b64 numba_attempt_nocopy_reshape_param_0,
        .param .b64 numba_attempt_nocopy_reshape_param_1,
        .param .b64 numba_attempt_nocopy_reshape_param_2,
        .param .b64 numba_attempt_nocopy_reshape_param_3,
        .param .b64 numba_attempt_nocopy_reshape_param_4,
        .param .b64 numba_attempt_nocopy_reshape_param_5,
        .param .b64 numba_attempt_nocopy_reshape_param_6,
        .param .b32 numba_attempt_nocopy_reshape_param_7
)

There are some mismatches in the size of parameters, where they are 32-bit in the implementation but 64-bit from Numba's perspective.

*/
#define NPY_MAXDIMS 32

typedef long int npy_intp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect that this should be something other than a long int on Windows.

This is 64 bits on Linux and Windows, which corrects an issue with the
prototypes not matching on Windows.
@gmarkall
Copy link
Collaborator

/ok to test

@gmarkall
Copy link
Collaborator

Using long long int seemed to work on Windows, so I've pushed it to ensure it's still OK on Linux.

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review Ready for review by team labels Nov 29, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the tests are fine, but they should be in test_array.py rather than test_cuda_array_interface.py. test_cuda_array_interface.py is for testing the implementation of the CUDA Array Interface.

@@ -430,6 +455,55 @@ def f(x, y):
# Ensure that synchronize was not called
mock_sync.assert_not_called()

# @skip_unless_cuda_python('NVIDIA Binding needed for NVRTC')
Copy link
Collaborator

Choose a reason for hiding this comment

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

When moving the tests, I think this comment can be removed, as it pertained to an old version of Numba.

Copy link
Collaborator

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of small suggestions on moving the tests.

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Dec 2, 2024
@gmarkall gmarkall modified the milestones: v0.0.20, v0.0.21, v0.0.22 Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on author Waiting for author to respond to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants