-
Notifications
You must be signed in to change notification settings - Fork 55
[RFC,TESTING] Add filecheck test infrastructure #342
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
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
isVoid
left a comment
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'm trying to understand the use of this tool. Also a broader question, the result of the compiler are in fact legal nvvm IRs, should we use NVVM as the prefix instead?
Sure! Whatever will most precisely differentiate the checks from one another. |
|
@isVoid I tried answering all your questions, but let me speak about the motivation more generally: FileCheck allows us to easily create meaningful tests (in a semi-automated way) to increase the test coverage we have so we feel more secure making larger changes. Consider this test in LLVM Flang. There are many ways to compile the same test, some of which result in the same IR. The same source code can be reused to create 6 or so test cases, some of which share the same checks. There are scripts in the LLVM project to generate and update these tests automatically. When we vendor in components for the purpose of cuda-specific extensions, instead of vendoring in the unit tests from numba core into numba cuda directly, we can vendor in the kernels and use This is also just a suggestion/RFC. We will discuss this further before submitting any changes. Thank you for the review and please ask more questions if I left anything unclear! |
gmarkall
left a comment
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 really like the idea here! This looks like a nice way to check the generated code.
I do think we could simplify the implementation and API, and make it a bit closer to the idioms of the rest of the test suite - see comments on the diff for these suggestions.
I also wonder - does FileCheck identify the failing line and provide error messages on stderr? I notice the Matcher class has an stderr member but I haven't dived deeper - I am looking to understand whether the framework can provide more precise error messages when failures occur.
It does - but please note that this is a Python re-implementation of the FileCheck executable program which is a part of the LLVM project, so I am also unfamiliar with the API of this Python module. I will investigate further and see how we can make this more ergonomic. Thank you for the review! |
Adding pattern-based checking infrastructure allows us to describe the IR and assembly expected under different conditions and expressed right next to the code that produced it. This is closer to the LLVM testing standards, and will help is iterate more quickly and add test coverage while we are in the process of vendoring in components from numba core. This is a request for comment, as filecheck tests will be meaningfully different from existing numba-cuda tests.
0146b43 to
398233c
Compare
|
/ok to test c45924d |
|
/ok to test df00dc7 |
|
/ok to test 0552773 |
|
@ashermancinelli thanks for the explanation. When we write tests, we are limited by the tool that we have so that we can't test more complicated pattern. This opens up a whole range of options for us. Just a simple grab through the code base with Details |
Co-authored-by: Graham Markall <[email protected]>
numba_cuda/numba/cuda/testing.py
Outdated
| self, | ||
| ir_producer: CUDADispatcher, | ||
| signature: tuple[type, ...] | None = None, | ||
| check_prefixes: list[str] = ["ASM"], |
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 use of a list as a default argument is probably safe as it's not mutated, but it always sets off alarm bells to see a mutable default in a Python function definition. I think you could either use None as in https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments, or have a tuple for the default instead (I assume filecheck will accept a tuple as well).
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.
A tuple or frozenset would be okay with me, let me try that out. Thanks!
|
/ok to test 8d6dcb8 |
|
The filecheck module compatible with python 3.9 is too old to be used here. |
|
/ok to test 8fb7920 |
|
/ok to test dd6eb32 |
|
/ok to test |
|
I think the PR is good. I am just running another check with a merge of |
- [NFC] FileCheck tests check all overloads (NVIDIA#354) - [REVIEW][NFC] Vendor in serialize to allow for future CUDA-specific refactoring and changes (NVIDIA#349) - Vendor in usecases used in testing (NVIDIA#359) - Add thirdparty tests of numba extensions (NVIDIA#348) - Support running tests in parallel (NVIDIA#350) - Add more debuginfo tests (NVIDIA#358) - [REVIEW][NFC] Vendor in the Cache, CacheImpl used by CUDACache and CUDACacheImpl to allow for future CUDA-specific refactoring and changes (NVIDIA#334) - [NFC] Vendor in Dispatcher as CUDADispatcher to allow for future CUDA-specific customization (NVIDIA#338) - Vendor in BaseNativeLowering and BaseLower for CUDA-specific customizations (NVIDIA#329) - [REVIEW] Vendor in the CompilerBase used by CUDACompiler to allow for future CUDA-specific refactoring and changes (NVIDIA#322) - Vendor in Codegen and CodeLibrary for CUDA-specific customization (NVIDIA#327) - Disable tests that deadlock due to NVIDIA#317 (NVIDIA#356) - FIX: Add type check for shape elements in DeviceNDArrayBase constructor (NVIDIA#352) - Merge pull request NVIDIA#265 from lakshayg/fp16-support - Add performance warning - Fix tests - Create and register low++ bindings for float16 - Create typing/target registries for float16 - Replace Numbast generated lower_casts - Replace Numbast generated operators - Alias __half to numba.core.types.float16 - Generate fp16 bindings using numbast - Remove existing fp16 logic - [REVIEW][NFC] Vendor in the utils and cgutils to allow for future CUDA-specific refactoring and changes (NVIDIA#340) - [RFC,TESTING] Add filecheck test infrastructure (NVIDIA#342) - Migrate test infra to pytest (NVIDIA#347) - Add .vscode to gitignore (NVIDIA#344) - [NFC] Add dev dependencies to project config (NVIDIA#341) - Allow Inspection of Link-Time Optimized PTX (NVIDIA#326) - [NFC] Vendor in DIBuilder used by CUDADIBuilder (NVIDIA#332) - Add guidance on setting up pre-commit (NVIDIA#339) - [Refactor][NFC] Vendor in MinimalCallConv (NVIDIA#333) - [Refactor][NFC] Vendor in BaseCallConv (NVIDIA#324) - [REVIEW] Vendor in CompileResult as CUDACompileResult to allow for future CUDA-specific customizations (NVIDIA#325)
- [NFC] FileCheck tests check all overloads (#354) - [REVIEW][NFC] Vendor in serialize to allow for future CUDA-specific refactoring and changes (#349) - Vendor in usecases used in testing (#359) - Add thirdparty tests of numba extensions (#348) - Support running tests in parallel (#350) - Add more debuginfo tests (#358) - [REVIEW][NFC] Vendor in the Cache, CacheImpl used by CUDACache and CUDACacheImpl to allow for future CUDA-specific refactoring and changes (#334) - [NFC] Vendor in Dispatcher as CUDADispatcher to allow for future CUDA-specific customization (#338) - Vendor in BaseNativeLowering and BaseLower for CUDA-specific customizations (#329) - [REVIEW] Vendor in the CompilerBase used by CUDACompiler to allow for future CUDA-specific refactoring and changes (#322) - Vendor in Codegen and CodeLibrary for CUDA-specific customization (#327) - Disable tests that deadlock due to #317 (#356) - FIX: Add type check for shape elements in DeviceNDArrayBase constructor (#352) - Merge pull request #265 from lakshayg/fp16-support - Add performance warning - Fix tests - Create and register low++ bindings for float16 - Create typing/target registries for float16 - Replace Numbast generated lower_casts - Replace Numbast generated operators - Alias __half to numba.core.types.float16 - Generate fp16 bindings using numbast - Remove existing fp16 logic - [REVIEW][NFC] Vendor in the utils and cgutils to allow for future CUDA-specific refactoring and changes (#340) - [RFC,TESTING] Add filecheck test infrastructure (#342) - Migrate test infra to pytest (#347) - Add .vscode to gitignore (#344) - [NFC] Add dev dependencies to project config (#341) - Allow Inspection of Link-Time Optimized PTX (#326) - [NFC] Vendor in DIBuilder used by CUDADIBuilder (#332) - Add guidance on setting up pre-commit (#339) - [Refactor][NFC] Vendor in MinimalCallConv (#333) - [Refactor][NFC] Vendor in BaseCallConv (#324) - [REVIEW] Vendor in CompileResult as CUDACompileResult to allow for future CUDA-specific customizations (#325)
Adding pattern-based checking infrastructure allows us to describe the IR and assembly expected under different conditions right next to the code that produces it. This is closer to the LLVM testing standards, and will help is iterate more quickly and add test coverage while we are in the process of vendoring in the components from numba core we need to modify in a cuda-specific way.
This PR changes a couple tests to use the filecheck patterns. For example, in numba-cuda today there tests like
self.assertIn('function_name', llvm_ir). Now, we can express the same check in the docstring of the kernel, and all the checks we would like to perform may live inside the kernel they come from.This is a request-for-comment, as filecheck tests will be meaningfully different from existing numba-cuda tests.
Note also that the filecheck that has been added to the list of dependencies is not the filecheck that is compiled as a part of the LLVM project, but is an attempt to replicate the functionality in a pure python package.
I ran some tests to see how well the usual FileCheck features work with the Python version, and it seems okay.
If we are uncomfortable with this development dependency, we can reevaluate.
TODO:
CUDADispatcherwhen running with the simulatorAnother solution I considered would be to structure our tests much more like LLVM lit tests, not just using the filecheck pattern like I did in this patch.
In this alternative, I imagine test kernels would be written in their own files: