Skip to content

Fix dubious find_package logic in test/generator#8804

Merged
abadams merged 4 commits intomainfrom
bugfix/suspicious-opencl-import
Sep 15, 2025
Merged

Fix dubious find_package logic in test/generator#8804
abadams merged 4 commits intomainfrom
bugfix/suspicious-opencl-import

Conversation

@alexreinking
Copy link
Member

Fixes #8790

@alexreinking alexreinking marked this pull request as ready for review September 12, 2025 01:10
@alexreinking
Copy link
Member Author

The failing ruff check is fixed in #8803

@abadams
Copy link
Member

abadams commented Sep 12, 2025

I'm not sure I follow the original or new logic. HL_TARGET containing "cuda" definitely doesn't imply any build-time dependence on anything cuda-related, toolkit or driver. But I see there's an explicit include of cuda.h in test/common/gpu_context.h and this is included by acquire_release_aottest.cpp. So this is usage of the cuda toolkit that happens independently from Halide's use of cudaI

Is sniffing HL_TARGET just a proxy for tests that might potentially depend on the toolkit? Would it be better/clearer to just explicitly declare that those specific tests depend on the toolkit? I think it's just acquire_release_aottest.cpp and gpu_multi_context_threaded_aottest.cpp. For opencl, it's those two plus define_extern_opencl_aottest.cpp

@alexreinking
Copy link
Member Author

I'm not sure I follow the original or new logic. HL_TARGET containing "cuda" definitely doesn't imply any build-time dependence on anything cuda-related, toolkit or driver. But I see there's an explicit include of cuda.h in test/common/gpu_context.h and this is included by acquire_release_aottest.cpp. [...] Would it be better/clearer to just explicitly declare that those specific tests depend on the toolkit? I think it's just acquire_release_aottest.cpp and gpu_multi_context_threaded_aottest.cpp. For opencl, it's those two plus define_extern_opencl_aottest.cpp

It would be cleaner; I updated the PR.

So this is usage of the cuda toolkit that happens independently from Halide's use of cuda!

nit: It's not independent because the header is only included by gpu_context.h when TEST_CUDA (say) is set, and this is only set when cuda is present in Halide_TARGET (similarly for OpenCL).

@abadams abadams merged commit 0c4593d into main Sep 15, 2025
16 of 17 checks passed
@alexreinking alexreinking deleted the bugfix/suspicious-opencl-import branch September 15, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suspicious CMake code for generator tests.

2 participants