-
Notifications
You must be signed in to change notification settings - Fork 238
test_nvvm.py simplification / use llvmlite in toolshed/ only
#1047
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
Changes from all commits
bdd4887
35b0a6d
4079a55
b054e9e
1019685
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| #!/usr/bin/env python3 | ||
|
|
||
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. | ||
| # SPDX-License-Identifier: LicenseRef-NVIDIA-SOFTWARE-LICENSE | ||
|
|
||
| """ | ||
| Helper to produce static bitcode input for test_nvvm.py. | ||
| Usage: | ||
| python toolshed/build_static_bitcode_input.py | ||
| It will print a ready-to-paste MINIMAL_NVVMIR_BITCODE_STATIC entry for the | ||
| current NVVM IR version detected at runtime. | ||
| """ | ||
|
|
||
| import binascii | ||
| import os | ||
| import sys | ||
| import textwrap | ||
|
|
||
| import llvmlite.binding # HINT: pip install llvmlite | ||
| from cuda.bindings import nvvm | ||
|
|
||
|
|
||
| def get_minimal_nvvmir_txt_template(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you just check in a JSON file that contains the bitcode? Why do we need the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where would you put the JSON file? I had on my mind: Avoid creating extra artifacts. Re printing out the generated dictionary: I don't want to over-engineer a helper script that's used only rarely (possibly only every couple years). The main goal is to archive "how it worked last time", so that future maintainers don't have to start from scratch. (Back in February it took me more than a couple hours to figure out the approach.) |
||
| cuda_bindings_tests_dir = os.path.normpath("cuda_bindings/tests") | ||
| assert os.path.isdir(cuda_bindings_tests_dir), ( | ||
| "Please run this helper script from the cuda-python top-level directory." | ||
| ) | ||
| sys.path.insert(0, os.path.abspath(cuda_bindings_tests_dir)) | ||
| import test_nvvm | ||
|
|
||
| return test_nvvm.MINIMAL_NVVMIR_TXT_TEMPLATE | ||
|
|
||
|
|
||
| def main(): | ||
| major, _minor, debug_major, _debug_minor = nvvm.ir_version() | ||
| txt = get_minimal_nvvmir_txt_template() % (major, debug_major) | ||
| bitcode_dynamic = llvmlite.binding.parse_assembly(txt.decode()).as_bitcode() | ||
| bitcode_hex = binascii.hexlify(bitcode_dynamic).decode("ascii") | ||
| print("\n\nMINIMAL_NVVMIR_BITCODE_STATIC = { # PLEASE ADD TO test_nvvm.py") | ||
| print(f" ({major}, {debug_major}): # (major, debug_major)") | ||
| lines = textwrap.wrap(bitcode_hex, width=80) | ||
| for line in lines[:-1]: | ||
| print(f' "{line}"') | ||
| print(f' "{lines[-1]}",') | ||
| print("}\n", flush=True) | ||
| print() | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| assert len(sys.argv) == 1, "This helper script does not take any arguments." | ||
| main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow why we can't make
llvmlitea required testing dependency and always generate the bitcode and eliminate the static testing fixture altogether.Then we can avoid a lot of this complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mutually exclusive with my JSON file suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's whole point of this PR: remove
llvmliteeven as an optional test dependency.@leofang please correct me if I'm wrong, but I got the impression you were skeptical of the
llvmlitedependency all the while.Originally I wanted to keep things simple and make
llvmlitea required dependency.Before we had the recent
llvmlitev0.45related breakage (see #988 and numba/llvmlite#1297), I had it on my list to simplify thetest_nvvm.pycode, withllvmliteas a hard dependency.But after the breakage, and seeing how @rparolin stumbled even over the optional dependency last week (see team chat), I got to think it's more trouble than it's worth to even have it as an optional dependency.
With this PR, the
cuda_bindingsunit tests will be fully isolated fromllvmlite, so we won't stumble that much in the future.Only every couple years probably someone has to dust off the helper script to add a new entry in
test_nvvm.py. Estimated effort: 30-60 minutes, depending on prior background of the person who takes this on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't we still need
llvmliteeventually to regenerate the bitcode?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but only "offline", in the sense that the routine
cuda_bindingsunit tests (CI) don't need it.Based on the few months of related experience that I have:
test_nvvm.pywill only break if we're testing new CTK releases.test_nvvm.pywill be just one of potentially several adjustments we have to make for new CTK versions.test_nvvm.pybreakages due to minor-version CTK releases are probably small. — Every couple of months roughly.test_nvvm.pybreakages due to major-version CTK releases are significant. — Every couple years.Making
llvmlitea required dependency means that we'll be living at the bleeding edge: random breakages at random times (from NVIDIA's viewpoint) that need immediate attention.With the static bitcode inputs, we'll only need to tend to the test when there are CTK changes (release schedule controlled by NVIDIA).
For completeness: There are other ways to convert the
txtversion tobitcode. I don't remember exactly, I believe there are llvm commands that could be used instead. What's most suitable might change in the future, but I'm guessing as long asnumba-cudausesllvmlite, it'll be a good choice.