[NVVM] Support - Followup enhancements #1218
Conversation
leofang
added
enhancement
|
Thanks, @abhilash1910! Any ETA to wrap this up? |
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
leofang
left a comment
leofang
left a comment
There was a problem hiding this comment.
Thanks, Abhilash! Leaving a few early feedbacks.
cuda_core/tests/test_program.py
Outdated
| bitcode_path = os.environ.get("BITCODE_NVVM_PATH") | ||
| if not bitcode_path: | ||
| pytest.skip("BITCODE_NVVM_PATH environment variable is not set.Disabling the test.") | ||
| bitcode_file = Path(bitcode_path) | ||
| if not bitcode_file.exists(): | ||
| pytest.skip(f"Bitcode file not found: {bitcode_path}") | ||
|
|
||
| if bitcode_file.suffix != ".bc": | ||
| pytest.skip(f"Expected .bc file, got: {bitcode_file.suffix}") |
There was a problem hiding this comment.
Would it be possible for us to avoid having a file locally? We have bitcode in this repo already:
cuda-python/cuda_bindings/tests/test_nvvm.py
Lines 12 to 141 in b9c76b3
so I suggest that we move it to the common place, say a new file under
cuda_python_test_helpers:https://github.com/NVIDIA/cuda-python/tree/main/cuda_python_test_helpers/cuda_python_test_helpers
and have it imported in both cuda.bindings/core tests.
There was a problem hiding this comment.
Agree on this.
There was a problem hiding this comment.
This should be partially addressed now as I have yet to remove the existing invocations from cuda_bindings test
| extra_name = f"{options.name}_extra_{i}" | ||
| nvvm.add_module_to_program(self._mnff.handle, extra_source, len(extra_source), extra_name) |
There was a problem hiding this comment.
Two notes:
- I am torn if we should allow users to specify the module names, instead of us making one up for them. Could you try my "sequence of 2-tuples" suggestion from our offline chat, and see how bad the code looks like? We should have something similar already in one of the options.
- As discussed let's check if all modules can be lazily loaded.
There was a problem hiding this comment.
- is done
- if we use lazy mode always then we have to remove textual IR as input as lazy mode always supports bitcode format.
@leofang what do you suggest?
There was a problem hiding this comment.
Do you mean that the lazy loading does not support text IR, only bitcode IR?
There was a problem hiding this comment.
yes I think so , looking at the error
|
pre-commit.ci autofix |
|
FYI, there are some merge conflicts due to #1351 that need to be resolved 😅 |
|
@rwgk @rparolin Could you share feedbacks with @abhilash1910 with regard to the changes on the pathfinder? Let's make sure this PR can be merged for cuda-core v0.6.0. |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
It's not ideal to add find_libdevice under _dynamic_libs. What do people think about introducing _static_libs? This would be for .a and .bc files.
Just noting: the found_via feature is missing, but we can do that in a follow-on PR.
We'll also need pathfinder tests, but I can help with that in a follow-on PR.
| ) | ||
|
|
||
|
|
||
| class LibdeviceNotFoundError(Exception): |
There was a problem hiding this comment.
For consistency with DynamicLibNotFoundError (in load_dl_common.py):
class LibdeviceNotFoundError(RuntimeError):
| pass | ||
|
|
||
|
|
||
| def _get_cuda_home_or_path() -> str | None: |
There was a problem hiding this comment.
Could you please use cuda.pathfinder._utils.get_cuda_home_or_path?
|
|
||
|
|
||
| class _FindLibdevice: | ||
| FILENAME = "libdevice.10.bc" |
There was a problem hiding this comment.
Could you please move this up to the top of the file, to make it more obvious that we have this hard-wired?
| if IS_WINDOWS: | ||
| bases = [r"C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA", r"C:\CUDA"] | ||
| else: | ||
| bases = ["/usr/local/cuda", "/opt/cuda"] |
There was a problem hiding this comment.
Style: Could you please also move this up to the top?
More fundamental:
Thus far we've tried hard to avoid hard-coding these paths. It's of course very easy for us, but can also easily backfire if there are multiple CUDA installations. We should probably make sure @kkraus14 is on board with this. (If he is I'm fine with it, too.)
| if os.path.isfile(file_path): | ||
| return file_path | ||
| # Versioned paths (e.g., /usr/local/cuda-13.0) | ||
| for versioned in sorted(glob.glob(base + "*"), reverse=True): |
There was a problem hiding this comment.
We're not doing this for dynamic libs or headers. We could wind up with a weird mix of versions between dynamic libs, headers, and libdevice if we don't have a general mechanism for deciding "What is the system installation?" I fear if that happens, the consequences will be subtle but potentially extremely confusing and time consuming to troubleshoot. @kkraus14 again for guidance.
| def find_libdevice() -> str: | ||
| """Find the path to libdevice.10.bc. | ||
| Raises: | ||
| LibdeviceNotFoundError: If libdevice.10.bc cannot be found |
There was a problem hiding this comment.
LibdeviceNotFoundError: If the libdevice*.bc file cannot be found
(To avoid hard-wiring the name multiple times.)
| return abs_path | ||
|
|
||
|
|
||
| def get_libdevice_path() -> str | None: |
There was a problem hiding this comment.
We should definitely make this DRY and find naming that's intuitive in the call sites; i.e. people shouldn't need to come back here to figure out the difference between find/get.
My preferred solution would be
def find_libdevice(optional: bool = False) -> str | None:
Simple, just a tiny little bit weaker for type checkers.
| is_culink_backend = _linker._decide_nvjitlink_or_driver() | ||
|
|
||
| try: | ||
| from cuda_python_test_helpers.nvvm_bitcode import minimal_nvvmir |
There was a problem hiding this comment.
I think we can make this import unconditional. At least that's how I would start, then add except ModuleNotFoundError (instead of ImportError, which can mask all sort of bugs) if we really need it.
3 participants
Description
Issue Link - #981
Changes to be addressed in this WIP PR:
{If / when it is possible to add multiple modules, a test with code that uses something from libdevice is probably a good idea.
It's also useful to be able to lazily add a module}
cc @leofang