[NVVM] Support - Followup enhancements #1218
Conversation
|
Thanks, @abhilash1910! Any ETA to wrap this up? |
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
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?
|
pre-commit.ci autofix |
|
FYI, there are some merge conflicts due to #1351 that need to be resolved 😅 |
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