Implement missing Makefile features in CMake#9227
Merged
Merged
Conversation
This app built standalone driver binaries by linking the per-op object files and headers that the simd_op_check correctness test emitted into its output directory. That test now JITs each case via compile_to_callable and emits only assembly, so the objects and headers the driver links against are no longer produced and the app can no longer be built. Fixes #8741 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #8740 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Also cast the resampling kernel's tap count to int before using it as an RDom extent, which must be integral. Fixes #8732 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Also fix two problems that kept the pipelines from building: the output copy_to_device() calls defaulted to the GPU device API, so pin them to HexagonDma; and the YUV harness called embed() on statically-2-D Runtime::Buffers, so give the chroma planes dynamic dimensionality. Fixes #8734 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build the Halide ops with pytorch_wrapper output and synthesize the pybind extension in CMake instead of setup.py. The app configures only when Python and PyTorch are found, taking the extension's include and library paths from torch.utils.cpp_extension so it works against a CUDA-enabled wheel without a matching standalone CUDA toolkit. Its CUDA ops are gated behind HELLO_PYTORCH_CUDA. Fixes #8729 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The nn_ops app is no longer present in the tree, so drop its dangling commented-out add_app() TODO from apps/CMakeLists.txt. Fixes #8736 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Provide CMake-native equivalents for rerunning the test suite under an external tool, using CTest's own mechanisms rather than bespoke scripting. Valgrind maps onto CTest's MemCheck action. A new Halide_ENABLE_MEMCHECK option pulls in the CTest module (so `ctest -T memcheck` works in the build tree with no dashboard submission) and configures the memory checker via MEMORYCHECK_COMMAND_OPTIONS and MEMORYCHECK_SUPPRESSIONS_FILE (test/valgrind.supp). The options omit --leak-check=full on purpose: Halide's JIT triggers one-time LLVM ORC allocations that valgrind reports as "possibly lost", which are noise rather than Halide bugs. correctness_tracing_stack does an intentional out-of-bounds read, so it is tagged with a no_memcheck label and excluded via -LE no_memcheck. Intel SDE maps onto CMAKE_CROSSCOMPILING_EMULATOR, which prefixes every add_test-registered command with the emulator, so no per-test enumeration is needed. This works at the current 3.28 CMake floor. New presets drive both: `valgrind` (the test preset applies the label exclusion; add -T memcheck at the ctest invocation) and `avx512-cannonlake` / `avx512-knights-landing` (sde -cnl -- / sde -knl --). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give the six benchmark apps (bilateral_grid, camera_pipe, lens_blur, local_laplacian, nl_means, stencil_chain) a CMake-native benchmark path. Each app now emits a REGISTRATION source from its primary filter, builds an <app>_rungen executable (linking Halide::RunGenMain + the filter, following the existing apps/HelloWasm pattern), and registers an <app>_benchmark CTest that runs the filter with --benchmarks=all --estimate_all --parsable_output. Every benchmark test is labeled benchmark_apps, so `ctest -L benchmark_apps` runs all of them at once. The label is benchmark_apps rather than plain benchmark because ctest -L matches labels as a regex: a bare `benchmark` would also select the unrelated hexagon_benchmarks app, whose label contains that substring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The build-from-source Hexagon remote runtime (Halide_BUILD_HEXAGON_REMOTE_RUNTIME) installed its artifacts to a flat bin/, so its output did not match the committed src/runtime/hexagon_remote/bin/ subtree. Fix the install destinations so the runtime can be rebuilt from source and installed straight back over the committed blobs: - qurt artifacts -> bin/<arch>, where <arch> is the toolchain's HEXAGON_ARCH (e.g. v65), guarded against being unset rather than hardcoded. - android host libs -> bin/arm-<bits>-android, <bits> from the NDK toolchain's pointer size. - the simulator-host lib -> bin/host (it previously had no install rule). - the qaic-generated IDL sources -> bin/src. - everything tagged COMPONENT Halide_Hexagon, destinations under CMAKE_INSTALL_BINDIR (GNUInstallDirs). The qurt/android pieces are ExternalProjects, whose install steps run at build time and which `cmake --install` does not descend into, so they now install into one shared staging tree that a single install(DIRECTORY ... TYPE BIN) relays into the real prefix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9227 +/- ##
=======================================
Coverage ? 70.19%
=======================================
Files ? 255
Lines ? 78939
Branches ? 18882
=======================================
Hits ? 55408
Misses ? 17897
Partials ? 5634 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… on MSVC CI configures the ambient Halide_TARGET to GPU-testing targets like host-cuda when exercising the apps test label, and since this app's add_halide_library calls didn't specify TARGETS, they inherited it, producing e.g. host-cuda-hexagon_dma. That combination JIT-compiles the whole combined runtime just to auto-detect the host's CUDA compute capability, which then fails to link against hexagon_dma's mock-only driver symbols. These pipelines never touch any GPU device API, so pin them to a plain host target instead. Also skip the mock_dma_implementation.cpp-based executables and tests under MSVC, since its WEAK == __attribute__((weak)) macro doesn't compile there.
auto_viz's pipelines are built with trace_all, but auto_viz_demo.cpp never sets HL_TRACE_FILE, so Halide's default trace handler falls back to formatting every load/store/produce/consume event as plain text via halide_print. For even the small test image that's tens of millions of lines (multiple GB) of stdout. Direct execution is merely slow, but a test runner that captures/streams that output (e.g. `ctest -V`, as the buildbot uses) can burn through the entire test timeout on it. Point HL_TRACE_FILE at the null device for the auto_viz_demo test so it exercises the same binary trace-writing path (still a meaningful check that trace_all pipelines run) while discarding the output almost instantly.
alexreinking
force-pushed
the
alexreinking/makefile-parity
branch
from
July 21, 2026 17:21
e9b52ca to
2a445e0
Compare
shoaibkamil
reviewed
Jul 22, 2026
shoaibkamil
reviewed
Jul 22, 2026
shoaibkamil
reviewed
Jul 22, 2026
shoaibkamil
reviewed
Jul 22, 2026
The down row of the dispatch table pointed at auto_viz_demo_complex_up instead of _down. Expand the CTest coverage to all six (schedule, direction) variants; per discussion with @shoaibkamil, that alone can't catch this class of bug since the program still exits 0 with a valid image either way -- verifying output correctness is tracked as separate future work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- HelloPyTorch/CMakeLists.txt: PYTORCH_WRAPPER names a variable to receive the generated header's path, not a filename. Add add_pytorch_op() to capture it properly and auto-populate op_libs and the wrapper #include list, instead of relying on filename convention. - Clarify why CMakeLists.txt doesn't probe for a valgrind binary itself (CTest's memcheck driver already handles that). - Tighten the valgrind test preset's description. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Nothing about the loop needs explaining beyond what the code already says; the comment only made sense as a rationale tied to a bug that's no longer relevant to the present code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shoaibkamil
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We've had a few gaps in our CMake coverage for a long time. I finally added stubs back in #9093 and drive-by filled in the remaining stub generator tests in #9205. There were only a few gaps that I could find remaining:
auto_viz,HelloPyTorch,hexagon_dma,nn_ops,resnet_50, andsimd_op_checkbilateral_grid,camera_pipe,lens_blur,local_laplacian,nl_means, andstencil_chaininstall_qcHexagon target.This PR fills in all these gaps.
auto_viz,HelloPyTorch,hexagon_dma, andresnet_50nn_opsapp was removed ages ago, so I remove a stale comment.simd_op_checkapp because it relied oncorrectness_simd_op_checkproducing object files it no longer produces.Fixes #8729
Fixes #8732
Fixes #8734
Fixes #8736
Fixes #8740
Fixes #8741
Checklist