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

Soft link NVRTC for cupy_backends.cuda.libs.nvrtc #7621

Merged
merged 20 commits into from
Aug 30, 2023

Conversation

kmaehashi
Copy link
Member

@kmaehashi kmaehashi commented Jun 9, 2023

Part of #7620.

This PR changes the cupy_backends.cuda.libs.nvrtc module to load NVRTC symbols dynamically at runtime, instead of build-time linking.

Pros:

  • Make the module buildable & importable without NVRTC installed. (note: Jitify module still links to NVRTC, so this PR is not enough to run import cupy without NVRTC)
  • No longer need to maintain headers in cupy_backend.

Cons:

  • When calling NVRTC APIs, maintainers are responsible for passing the correct number/type of arguments, as the compiler can no longer detect such errors. Resolved by adding ctypedef for each API.

@emcastillo
Copy link
Member

This seems to be affecting rocm build somehow?

@emcastillo
Copy link
Member

/test mini

@kmaehashi kmaehashi marked this pull request as draft June 19, 2023 16:44
@kmaehashi
Copy link
Member Author

Will resume after #7647

@leofang
Copy link
Member

leofang commented Jul 5, 2023

Q: This might change the design significantly, but I wonder if it's possible to import the function signatures as is, and reinterpret the fetched symbol using the function signature? That is, instead of

     cdef F_t nvrtcCreateProgram = <F_t>_L.get('CreateProgram')

we do

    ctypedef int (*createProgramFn)(  # naming convention TBD
             Program* prog, const char* src, const char* name, int numHeaders,
             const char** headers, const char** includeNames)
    cdef createProgramFn nvrtcCreateProgram = < createProgramFn>_L.get('CreateProgram')

Basically, we'd be trading this pro

No longer need to maintain headers in cupy_backend.

with this con

When calling NVRTC APIs, maintainers are responsible for passing the correct number/type of arguments, as the compiler can no longer detect such errors.

The main blocker for this would be the maintainability of the Cython layer. We still have to keep the function signatures...

@kmaehashi
Copy link
Member Author

@leofang This is the part I was still wondering... "maintainers are responsible for passing the correct number/type of arguments" might be a bit dangerous as this immediately causes SegV on failure.

I'm now thinking that maintaining ctypedefs could be easier if we stop the convention of renaming typedefs (ctypedef nvrtcProgram Program). In that case, we can (almost) copy-paste the arguments from NVRTC header:

nvrtcResult nvrtcCreateProgram(nvrtcProgram *prog,
                               const char *src,
                               const char *name,
                               int numHeaders,
                               const char * const *headers,
                               const char * const *includeNames);

@leofang
Copy link
Member

leofang commented Jul 19, 2023

Sorry for some reason I missed your reply, Kenichi 😅

if we stop the convention of renaming typedefs

Yeah, sounds reasonable to me. I think at some point my hope was that all these are automated (say, #4395/#4427/#4606/#5876), and we can pick whatever style we like, but if it is no less work than this route, allowing easier copy/paste sounds like the right thing to do indeed.

@kmaehashi kmaehashi marked this pull request as ready for review August 12, 2023 09:15
@@ -1,4 +1,4 @@
[flake8]
filename = *.pyx, *.pxd, *.pxi
exclude = .git, .eggs, *.py
ignore = W503,W504,E225,E226,E227,E275,E402,E999
ignore = W503,W504,E211,E225,E226,E227,E275,E402,E999
Copy link
Member Author

Choose a reason for hiding this comment

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

E211 added to avoid Whitespace before '(' error:

ctypedef nvrtcResult (*F_nvrtcVersion)(int *major, int *minor) nogil
                    ^

@@ -36,6 +36,7 @@ repos:
rev: v0.15.0
hooks:
- id: cython-lint
args: ["--max-line-length", "79"]
Copy link
Member Author

@kmaehashi kmaehashi Aug 12, 2023

Choose a reason for hiding this comment

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

This is the same limitation as current coding standard (.flake8.cython)

Comment on lines +57 to +58
ctypedef nvrtcResult (*F_nvrtcCompileProgram)(
nvrtcProgram prog, int numOptions, const char** options) nogil
Copy link
Member Author

@kmaehashi kmaehashi Aug 12, 2023

Choose a reason for hiding this comment

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

ctypedefs can be generated by copy-pasting arguments from the header file.

nvrtc.h:

nvrtcResult nvrtcCompileProgram(nvrtcProgram prog,
                                int numOptions, const char * const *options);

@kmaehashi kmaehashi added this to the v13.0.0rc1 milestone Aug 12, 2023
@kmaehashi kmaehashi added the blocking Issue/pull-request is mandatory for the upcoming release label Aug 12, 2023
@kmaehashi kmaehashi marked this pull request as draft August 12, 2023 14:10
@kmaehashi kmaehashi marked this pull request as ready for review August 12, 2023 16:55
@kmaehashi
Copy link
Member Author

/test mini

@kmaehashi
Copy link
Member Author

/test cuda-python

@leofang
Copy link
Member

leofang commented Aug 29, 2023

LGTM!

@kmaehashi
Copy link
Member Author

Test passed!

if libname is None:
# Stub build or CUDA/HIP only library.
self.error = RuntimeError(
'The library is unavailable in the current platform.')
Copy link
Member

Choose a reason for hiding this comment

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

Nice!!

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM!!!!

@emcastillo emcastillo merged commit 96105af into cupy:main Aug 30, 2023
49 checks passed
@kmaehashi kmaehashi deleted the softlink-nvrtc branch August 30, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking Issue/pull-request is mandatory for the upcoming release cat:enhancement Improvements to existing features prio:high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants