[Target] Enforce invariants through setters - #9250
Open
alexreinking wants to merge 6 commits into
Open
Conversation
Extend validate_features() with more checks for blatantly incoherent targets, each enforced through the copy-validate-commit setters so they cannot be bypassed: - vector_bits requires a scalable-vector feature (SVE/SVE2/RVV/AVX10_1) - large_buffers requires a 64-bit target - profile and profile_by_timer are mutually exclusive - simulator requires the iOS OS - device sub-features require their parent (cuda capabilities require cuda, cl_* require opencl, vk_* require vulkan, hlsl_sm* require d3d12compute), via a shared sub_features_by_parent() table that subsumes the previous d3d12-specific check set_bits/set_vector_bits now validate, and the constructor sets vector_bits after features (its validity depends on them). All rules permit OSUnknown / bits==0 so freestanding and device sub-targets still construct. Enforcing the sub-feature rule made JITModule's per-feature GPU stripping throw (it cleared a parent while sub-features remained). Replace it with Target::without_device_features(), which clears every device-offload feature together with its sub-features. Covered by test/fuzz/target_invariants. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
EGL was a vestige of the removed OpenGL Compute support: nothing reads has_feature(EGL), so it had no codegen or runtime effect. Drop the enumerator and its Target::Feature, name-map, and Python-binding entries. Removing halide_target_feature_egl renumbers the subsequent halide_target_feature_t values, matching how earlier feature removals were handled (targets serialize by name, not by raw enum value). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add test/error coverage for the user_error/user_assert paths in Target.cpp that lacked dedicated tests, most of them the newly-added integrity checks: - feature incompatible with arch (sve2 on x86) - sub-feature without its parent (cuda_capability without cuda) - processor tuning invalid for arch (tune_znver4 on arm) - vector_bits without a scalable-vector feature - large_buffers on a 32-bit target - profile together with profile_by_timer - simulator without iOS - set_bits with an invalid width, set_vector_bits negative, set_feature with an out-of-range feature - natural_vector_size on a hexagon target without hvx - an unrecognized target-string token - no_bounds_query in HL_JIT_TARGET (skipped on Windows, no setenv) The has_unknowns bad_target_string path and natural_vector_size on an unknown target are already covered by incomplete_target.cpp and unknown_target.cpp. The remaining user_asserts are host CPUID probes (no SSE2 / no AltiVec) and the JIT host-mismatch check, which depend on the host/environment and aren't portably unit-testable. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
5 tasks
The new setter-enforced invariants surfaced three cases where existing code produced a now-invalid Target: - Dropping the last scalable-vector feature (e.g. deriving a non-SVE target from an SVE host, as warning/emulated_float16 does) left vector_bits orphaned. set_feature/set_features now clear it when no scalable-vector feature remains. - tutorial/lesson_11 reused one Target across cross-compiles, so x86 features lingered when the arch was switched to ARM. Start each cross-compile from a fresh Target. - realize_larger_than_two_gigs set large_buffers unconditionally; it can't run faithfully on a 32-bit target, so it now [SKIP]s there. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
alexreinking
marked this pull request as ready for review
July 29, 2026 02:00
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alexreinking/target-getters #9250 +/- ##
===============================================================
- Coverage 70.31% 70.22% -0.10%
===============================================================
Files 255 255
Lines 78916 79039 +123
Branches 18866 18887 +21
===============================================================
+ Hits 55491 55503 +12
- Misses 17832 17888 +56
- Partials 5593 5648 +55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Strengthen the contract on setters to ensure that
Target(t.to_string()) == tholds. Previously,tcould be pushed into an invalid state that theTarget()constructor would reject via its validation function.Add a fuzz tester for target invariants.
Drive-by: Removes unused EGL feature.
Breaking changes
Downstreams that construct illegal target strings will now see exceptions upon construction, rather than upon use.
Checklist
Stack created with GitHub Stacks CLI • Give Feedback 💬