From e0395e894ea65bd4477d9ce605c689acc48c47b2 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 24 Jul 2026 15:01:25 -0700 Subject: [PATCH 1/2] Centralize Target feature-flag implication logic Feature flags often imply each other: an AVX2 CPU necessarily also supports AVX, SSE41, F16C, and FMA, and there is no SVE2 device that isn't at least ARM v8.2-A. Until now the compiler dealt with these implications in two error-prone ways: - Inspecting a target by testing several flags at once, so that a query for "AVX512" had to enumerate every AVX512 variant that implies it. Miss one and the check is silently wrong. - Ad-hoc "complete the target" passes at the point of use (complete_x86_target, complete_arm_target), which hand-sequenced the implications. The ARM one got the order wrong: SVE/SVE2 set ARMFp16 only after the step that cascades ARMFp16 down to the v8.x baseline had already run, so a bare sve2 target ended up without ARMv8a/8.1a/8.2a and reported the wrong v8 lower bound. This centralizes the implications into a single topologically-ordered table on Target, with set_implied_features()/unset_implied_features()/ normalize() (plus with_/without_ copying variants). set walks the table forwards; unset walks it backwards to recover the minimal flag set. The table is the one source of truth, so a correctly-ordered single pass replaces both hand-written completion functions and fixes the SVE bug. Lowering calls set_implied_features() eagerly at the top, so every pass and the code generators inspect a fully-completed target and can check a single flag instead of a set. As a result the emitted Module carries a normalized Target: complete in code, but unset back to minimal form when printed (IRPrinter, StmtToHTML) so target strings stay compact. Also folds the now-redundant multi-flag checks (x86 AVX512/AVX chains, the tracing loads/stores implication) down to single-flag checks, and moves the Target self-tests out of libHalide into test/correctness/target.cpp. Co-Authored-By: Claude Opus 4.8 --- src/CodeGen_ARM.cpp | 73 +------------- src/CodeGen_LLVM.cpp | 5 + src/CodeGen_X86.cpp | 69 ++----------- src/IRPrinter.cpp | 3 +- src/Lower.cpp | 9 +- src/Pipeline.cpp | 5 +- src/StmtToHTML.cpp | 6 +- src/Target.cpp | 191 +++++++++++++++++++++++++----------- src/Target.h | 34 ++++++- src/Tracing.cpp | 7 +- test/correctness/target.cpp | 134 +++++++++++++++++++++++++ test/internal.cpp | 1 - 12 files changed, 328 insertions(+), 209 deletions(-) diff --git a/src/CodeGen_ARM.cpp b/src/CodeGen_ARM.cpp index dbc8458a999e..55236da3ecaa 100644 --- a/src/CodeGen_ARM.cpp +++ b/src/CodeGen_ARM.cpp @@ -35,77 +35,6 @@ using namespace llvm; namespace { -// Populate feature flags in a target according to those implied by -// existing flags, so that instruction patterns can just check for the -// oldest feature flag that supports an instruction. -// -// According to LLVM, ARM architectures have the following is-a-superset-of -// relationships: -// -// v9.5a > v9.4a > v9.3a > v9.2a > v9.1a > v9a; -// v v v v v -// v8.9a > v8.8a > v8.7a > v8.6a > v8.5a > v8.4a > ... > v8a; -// -// v8r has no relation to anything. -Target complete_arm_target(Target t) { - if (t.os == Target::OSX) { - // The Apple M1 implements the full ARM v8.4a spec. - t.set_feature(Target::ARMv84a); - } - - auto add_implied_feature_if_supported = [](Target &t, Target::Feature super, Target::Feature implied) { - if (t.has_feature(super)) { - t.set_feature(implied); - } - }; - - // ARMFp16 implies ARMv8.2-A; we don't know of any devices where - // that doesn't hold. The cascade loop below will set ARMv81a and ARMv8a. - add_implied_feature_if_supported(t, Target::ARMFp16, Target::ARMv82a); - - constexpr int num_arm_v8_features = 10; - static const Target::Feature arm_v8_features[num_arm_v8_features] = { - // The following loop depends on this array being sorted correctly. - // keep-sorted start numeric=yes order=desc - Target::ARMv89a, - Target::ARMv88a, - Target::ARMv87a, - Target::ARMv86a, - Target::ARMv85a, - Target::ARMv84a, - Target::ARMv83a, - Target::ARMv82a, - Target::ARMv81a, - Target::ARMv8a, - // keep-sorted end - }; - - for (int i = 0; i < num_arm_v8_features - 1; i++) { - add_implied_feature_if_supported(t, - arm_v8_features[i], - arm_v8_features[i + 1]); - } - - static const Target::Feature features_with_fp16[] = { - Target::SVE, - Target::SVE2, - }; - - for (const auto &f : features_with_fp16) { - add_implied_feature_if_supported(t, f, Target::ARMFp16); - } - - static const Target::Feature features_with_dotprod[] = { - Target::SVE2, - }; - - for (const auto &f : features_with_dotprod) { - add_implied_feature_if_supported(t, f, Target::ARMDotProd); - } - - return t; -} - // Substitute in loads that feed into slicing shuffles, to help with vld2/3/4 // emission. These are commonly lifted as lets because they get used by multiple // interleaved slices of the same load. @@ -301,7 +230,7 @@ class CodeGen_ARM : public CodeGen_CPU { }; CodeGen_ARM::CodeGen_ARM(const Target &target) - : CodeGen_CPU(complete_arm_target(target)) { + : CodeGen_CPU(target) { // TODO(https://github.com/halide/Halide/issues/8088): See if // use_llvm_vp_intrinsics can replace architecture specific code in this diff --git a/src/CodeGen_LLVM.cpp b/src/CodeGen_LLVM.cpp index 9263aef95f75..9b7b28157a95 100644 --- a/src/CodeGen_LLVM.cpp +++ b/src/CodeGen_LLVM.cpp @@ -203,6 +203,11 @@ void CodeGen_LLVM::set_context(llvm::LLVMContext &context) { } std::unique_ptr CodeGen_LLVM::new_for_target(const Target &target, llvm::LLVMContext &context) { + // Code generation inspects the target's features to decide which + // instructions are available, so it expects a target with all implied + // features already set (e.g. AVX2 implies AVX, SSE41, ...). This is + // guaranteed for the module produced by lower(), and for the host target + // used to compile JIT trampolines. std::unique_ptr result; if (target.arch == Target::X86) { result = new_CodeGen_X86(target); diff --git a/src/CodeGen_X86.cpp b/src/CodeGen_X86.cpp index 4ff685a7a4e2..98dd6a6d59a5 100644 --- a/src/CodeGen_X86.cpp +++ b/src/CodeGen_X86.cpp @@ -27,53 +27,6 @@ using namespace llvm; namespace { -// Populate feature flags in a target according to those implied by -// existing flags, so that instruction patterns can just check for the -// oldest feature flag that supports an instruction. -Target complete_x86_target(Target t) { - if (t.has_feature(Target::AVX10_1)) { - if (t.vector_bits >= 256) { - t.set_feature(Target::AVX2); - } - if (t.vector_bits >= 512) { - t.set_feature(Target::AVX512_SapphireRapids); - } - } - if (t.has_feature(Target::AVX512_SapphireRapids)) { - t.set_feature(Target::AVX512_Zen4); - t.set_feature(Target::AVXVNNI); - } - if (t.has_feature(Target::AVX512_Zen5)) { - t.set_feature(Target::AVX512_Zen4); - t.set_feature(Target::AVXVNNI); - } - if (t.has_feature(Target::AVX512_Zen4)) { - t.set_feature(Target::AVX512_Cannonlake); - } - if (t.has_feature(Target::AVX512_Cannonlake)) { - t.set_feature(Target::AVX512_Skylake); - } - if (t.has_feature(Target::AVX512_Cannonlake) || - t.has_feature(Target::AVX512_Skylake) || - t.has_feature(Target::AVX512_KNL)) { - t.set_feature(Target::AVX512); - } - if (t.has_feature(Target::AVX512)) { - t.set_feature(Target::AVX2); - } - if (t.has_feature(Target::AVX2)) { - t.set_feature(Target::AVX); - // All AVX2-enabled architectures have F16C and FMA - t.set_feature(Target::F16C); - t.set_feature(Target::FMA); - } - if (t.has_feature(Target::AVX)) { - t.set_feature(Target::SSE41); - } - - return t; -} - /** A code generator that emits x86 code from a given Halide stmt. */ class CodeGen_X86 : public CodeGen_CPU { public: @@ -121,7 +74,7 @@ class CodeGen_X86 : public CodeGen_CPU { }; CodeGen_X86::CodeGen_X86(Target t) - : CodeGen_CPU(complete_x86_target(t)) { + : CodeGen_CPU(t) { } const int max_intrinsic_args = 6; @@ -1734,7 +1687,7 @@ string CodeGen_X86::mcpu_target() const { } else if (target.has_feature(Target::AVX2)) { // x86-64-v3: SSE4.2, POPCNT, AVX, AVX2, BMI1/2, F16C, FMA, // LZCNT, MOVBE. Also covers AVX512 / AVX512_KNL, since both - // imply AVX2 (via complete_x86_target), but neither requires + // imply AVX2 (via set_implied_features), but neither requires // BW/DQ/VL which would come for free with v4. return "x86-64-v3"; } else if (target.has_feature(Target::AVX)) { @@ -1857,15 +1810,12 @@ string CodeGen_X86::mattrs() const { } // AVX512 features. Any AVX512 variant implies AVX2 (via - // complete_x86_target), so the mcpu baseline is at least + // set_implied_features), so the mcpu baseline is at least // x86-64-v3. Skylake-and-above selects x86-64-v4, which already // includes F/CD/BW/DQ/VL, but we still add those features // explicitly so the bare AVX512 / AVX512_KNL paths (which use // x86-64-v3) also get them. - if (target.has_feature(Target::AVX512) || - target.has_feature(Target::AVX512_KNL) || - target.has_feature(Target::AVX512_Skylake) || - target.has_feature(Target::AVX512_Cannonlake)) { + if (target.has_feature(Target::AVX512)) { attrs.emplace_back("+avx512f"); attrs.emplace_back("+avx512cd"); } @@ -1873,8 +1823,7 @@ string CodeGen_X86::mattrs() const { attrs.emplace_back("+avx512pf"); attrs.emplace_back("+avx512er"); } - if (target.has_feature(Target::AVX512_Skylake) || - target.has_feature(Target::AVX512_Cannonlake)) { + if (target.has_feature(Target::AVX512_Skylake)) { attrs.emplace_back("+avx512vl"); attrs.emplace_back("+avx512bw"); attrs.emplace_back("+avx512dq"); @@ -1931,13 +1880,9 @@ bool CodeGen_X86::use_soft_float_abi() const { int CodeGen_X86::native_vector_bits() const { if (target.has_feature(Target::AVX10_1)) { return target.vector_bits; - } else if (target.has_feature(Target::AVX512) || - target.has_feature(Target::AVX512_Skylake) || - target.has_feature(Target::AVX512_KNL) || - target.has_feature(Target::AVX512_Cannonlake)) { + } else if (target.has_feature(Target::AVX512)) { return 512; - } else if (target.has_feature(Target::AVX) || - target.has_feature(Target::AVX2)) { + } else if (target.has_feature(Target::AVX)) { return 256; } else { return 128; diff --git a/src/IRPrinter.cpp b/src/IRPrinter.cpp index 331e5de658b2..16cc4590e7e1 100644 --- a/src/IRPrinter.cpp +++ b/src/IRPrinter.cpp @@ -93,7 +93,8 @@ ostream &operator<<(ostream &stream, const Module &m) { stream << s << "\n"; } - stream << "module name=" << m.name() << ", target=" << m.target().to_string() << "\n"; + // The module retains implied features, but print it in minimal form. + stream << "module name=" << m.name() << ", target=" << m.target().without_implied_features().to_string() << "\n"; for (const auto &b : m.buffers()) { stream << b << "\n"; } diff --git a/src/Lower.cpp b/src/Lower.cpp index cd7ccc9a03f4..7a5c46caa41b 100644 --- a/src/Lower.cpp +++ b/src/Lower.cpp @@ -615,9 +615,14 @@ Module lower(const vector &output_funcs, const vector &requirements, bool trace_pipeline, const vector &custom_passes) { - Module result_module{strip_namespaces(pipeline_name), t}; + // Lowering and code generation inspect a target with all implied features + // set, so that (e.g.) a check for SSE41 succeeds on an AVX2 target. + // Normalize once here; the module retains the implied features, and is + // printed back in minimal form by unsetting them at the print sites. + Target target = t.with_implied_features(); + Module result_module{strip_namespaces(pipeline_name), target}; run_with_large_stack([&]() { - lower_impl(output_funcs, pipeline_name, t, args, linkage_type, requirements, trace_pipeline, custom_passes, result_module); + lower_impl(output_funcs, pipeline_name, target, args, linkage_type, requirements, trace_pipeline, custom_passes, result_module); }); return result_module; } diff --git a/src/Pipeline.cpp b/src/Pipeline.cpp index f4dca6ee3292..effa2c0aaa22 100644 --- a/src/Pipeline.cpp +++ b/src/Pipeline.cpp @@ -531,7 +531,10 @@ Module Pipeline::compile_to_module(const vector &args, const Module &old_module = contents->module; - bool same_compile = !old_module.functions().empty() && old_module.target() == target; + // A lowered module stores the target with implied features set, so compare + // against the same normalized form of the requested target. + bool same_compile = !old_module.functions().empty() && + old_module.target() == target.with_implied_features(); // Either generated name or one of the LoweredFuncs in the existing module has the same name. same_compile = same_compile && fn_name.empty(); bool found_name = false; diff --git a/src/StmtToHTML.cpp b/src/StmtToHTML.cpp index a40f3bfe51a6..89c88d8cf953 100644 --- a/src/StmtToHTML.cpp +++ b/src/StmtToHTML.cpp @@ -718,7 +718,8 @@ class HTMLCodePrinter : public IRVisitor { // -- print text print_opening_tag("span", "matched"); print_html_element("span", "keyword", "module"); - print_text(" name=" + m.name() + ", target=" + m.target().to_string()); + // The module retains implied features, but print it in minimal form. + print_text(" name=" + m.name() + ", target=" + m.target().without_implied_features().to_string()); print_closing_tag("span"); // Open code block to hold module body @@ -767,7 +768,8 @@ class HTMLCodePrinter : public IRVisitor { // -- print text print_opening_tag("span", "matched"); print_html_element("span", "keyword", "module"); - print_text(" name=" + m.name() + ", target=" + m.target().to_string()); + // The module retains implied features, but print it in minimal form. + print_text(" name=" + m.name() + ", target=" + m.target().without_implied_features().to_string()); print_closing_tag("span"); // Open code block to hold module body diff --git a/src/Target.cpp b/src/Target.cpp index 55f2978b869c..04e06e7370bc 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -1308,6 +1308,137 @@ void Target::set_features(const std::vector &features_to_set, bool valu } } +namespace { + +// The feature-implication table. Each entry {a, b} means "feature a implies +// feature b": there is no real device or configuration that has a set without +// b, so any target with a should be treated as also having b. The list is kept +// in topological order (an antecedent always appears before it is used as a +// consequent), so that a single forward pass sets every implied feature, and a +// single backward pass removes every redundant implied feature. +// +// Implications that depend on target state other than the feature set (the +// arch, os, or vector_bits) don't fit the simple pair model, and are handled +// directly in set_implied_features()/unset_implied_features(). +const std::vector> &implied_feature_pairs() { + static const std::vector> pairs = { + // x86. Each AVX-family feature is a strict superset of the ones below + // it, so it is impossible to have the higher one without the lower. + {Target::AVX512_SapphireRapids, Target::AVX512_Zen4}, + {Target::AVX512_SapphireRapids, Target::AVXVNNI}, + {Target::AVX512_Zen5, Target::AVX512_Zen4}, + {Target::AVX512_Zen5, Target::AVXVNNI}, + {Target::AVX512_Zen4, Target::AVX512_Cannonlake}, + {Target::AVX512_Cannonlake, Target::AVX512_Skylake}, + {Target::AVX512_Skylake, Target::AVX512}, + {Target::AVX512_KNL, Target::AVX512}, + {Target::AVX512, Target::AVX2}, + // Every AVX2-enabled architecture also has F16C and FMA. + {Target::AVX2, Target::F16C}, + {Target::AVX2, Target::FMA}, + {Target::AVX2, Target::AVX}, + {Target::AVX, Target::SSE41}, + + // ARM + {Target::SVE2, Target::ARMDotProd}, + {Target::SVE2, Target::ARMFp16}, + {Target::SVE, Target::ARMFp16}, + // ARMFp16 implies ARM v8.2-A; we don't know of any device where that + // doesn't hold. The v8.x cascade below then fills in v8.1a and v8a. + {Target::ARMFp16, Target::ARMv82a}, + // The ARM v8.x version features form a descending chain: each level + // implies the one below it, down to v8a. + {Target::ARMv89a, Target::ARMv88a}, + {Target::ARMv88a, Target::ARMv87a}, + {Target::ARMv87a, Target::ARMv86a}, + {Target::ARMv86a, Target::ARMv85a}, + {Target::ARMv85a, Target::ARMv84a}, + {Target::ARMv84a, Target::ARMv83a}, + {Target::ARMv83a, Target::ARMv82a}, + {Target::ARMv82a, Target::ARMv81a}, + {Target::ARMv81a, Target::ARMv8a}, + + // Tracing loads or stores also produces the enclosing realization + // begin/end events, so that the traced loads and stores have context. + {Target::TraceLoads, Target::TraceRealizations}, + {Target::TraceStores, Target::TraceRealizations}, + }; + return pairs; +} + +} // namespace + +void Target::set_implied_features() { + // Implications that depend on more than just the feature set. + if (arch == X86 && has_feature(AVX10_1)) { + // AVX10.1 at a given vector width supports the corresponding legacy + // AVX feature set. The pairs below then cascade further. + if (vector_bits >= 256) { + set_feature(AVX2); + } + if (vector_bits >= 512) { + set_feature(AVX512_SapphireRapids); + } + } + if (arch == ARM && os == OSX) { + // Apple silicon implements at least the ARM v8.4-A spec. + set_feature(ARMv84a); + } + + // Simple feature -> feature implications. One forward pass suffices because + // the table is topologically sorted. + for (const auto &[feature, implied] : implied_feature_pairs()) { + if (has_feature(feature)) { + set_feature(implied); + } + } +} + +void Target::unset_implied_features() { + // Walk the table backwards, clearing any feature that is implied by another + // feature that remains set. Because the table is topologically sorted, + // walking backwards guarantees a consequent is only cleared after it has + // been used as an antecedent, so a chain collapses to just its highest + // feature in one pass. + const auto &pairs = implied_feature_pairs(); + for (auto it = pairs.rbegin(); it != pairs.rend(); ++it) { + if (has_feature(it->first)) { + set_feature(it->second, false); + } + } + + // Undo the conditional implications from set_implied_features(). These run + // after the pair loop, mirroring how their seeds run before it there. + if (arch == X86 && has_feature(AVX10_1)) { + if (vector_bits >= 512) { + set_feature(AVX512_SapphireRapids, false); + } + if (vector_bits >= 256) { + set_feature(AVX2, false); + } + } + if (arch == ARM && os == OSX) { + set_feature(ARMv84a, false); + } +} + +void Target::normalize() { + set_implied_features(); + unset_implied_features(); +} + +Target Target::with_implied_features() const { + Target copy = *this; + copy.set_implied_features(); + return copy; +} + +Target Target::without_implied_features() const { + Target copy = *this; + copy.unset_implied_features(); + return copy; +} + bool Target::has_feature(Feature f) const { if (f == FeatureEnd) { return true; @@ -1968,64 +2099,4 @@ bool Target::get_runtime_compatible_target(const Target &other, Target &result) return true; } -namespace Internal { - -void target_test() { - Target t; - for (const auto &feature : feature_name_map) { - t.set_feature(feature.second); - } - for (int i = 0; i < (int)(Target::FeatureEnd); i++) { - internal_assert(t.has_feature((Target::Feature)i)) << "Feature " << i << " not in feature_names_map.\n"; - } - - // 3 targets: {A,B,C}. Want gcd(A,B)=C - std::vector> gcd_tests = { - {{"x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma"}}, - {{"x86-64-linux-sse41-fma-no_asserts-no_runtime", "x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma"}}, - {{"x86-64-linux-avx2-sse41", "x86-64-linux-sse41-fma", "x86-64-linux-sse41"}}, - {{"x86-64-linux-avx2-sse41", "x86-32-linux-sse41-fma", ""}}, - {{"x86-64-linux-cuda", "x86-64-linux", "x86-64-linux-cuda"}}, - {{"x86-64-linux-cuda-cuda_capability_50", "x86-64-linux-cuda", "x86-64-linux-cuda"}}, - {{"x86-64-linux-cuda-cuda_capability_50", "x86-64-linux-cuda-cuda_capability_30", "x86-64-linux-cuda-cuda_capability_30"}}, - {{"x86-64-linux-vulkan", "x86-64-linux", "x86-64-linux-vulkan"}}, - {{"x86-64-linux-vulkan-vk_v13", "x86-64-linux-vulkan", "x86-64-linux-vulkan"}}, - {{"x86-64-linux-vulkan-vk_v13", "x86-64-linux-vulkan-vk_v10", "x86-64-linux-vulkan-vk_v10"}}, - {{"hexagon-32-qurt-hvx_v65", "hexagon-32-qurt-hvx_v62", "hexagon-32-qurt-hvx_v62"}}, - {{"hexagon-32-qurt-hvx_v62", "hexagon-32-qurt", "hexagon-32-qurt"}}, - {{"hexagon-32-qurt-hvx_v62-hvx", "hexagon-32-qurt", ""}}, - {{"hexagon-32-qurt-hvx_v62-hvx", "hexagon-32-qurt-hvx", "hexagon-32-qurt-hvx"}}, - {{"x86-64-windows-d3d12compute-hlsl_sm66", "x86-64-windows-d3d12compute", "x86-64-windows-d3d12compute"}}, - {{"x86-64-windows-d3d12compute-hlsl_sm66", "x86-64-windows-d3d12compute-hlsl_sm60", "x86-64-windows-d3d12compute-hlsl_sm60"}}, - {{"x86-64-windows-d3d12compute-hlsl_sm62", "x86-64-windows-d3d12compute-hlsl_sm62", "x86-64-windows-d3d12compute-hlsl_sm62"}}, - {{"x86-64-windows-d3d12compute-hlsl_sm69", "x86-64-windows-d3d12compute", "x86-64-windows-d3d12compute"}}, - {{"x86-64-windows-d3d12compute-hlsl_sm69", "x86-64-windows-d3d12compute-hlsl_sm60", "x86-64-windows-d3d12compute-hlsl_sm60"}}, - }; - - for (const auto &test : gcd_tests) { - Target result{}; - Target a{test[0]}; - Target b{test[1]}; - if (a.get_runtime_compatible_target(b, result)) { - internal_assert(!test[2].empty() && result == Target{test[2]}) - << "Targets " << a.to_string() << " and " << b.to_string() << " were computed to have gcd " - << result.to_string() << " but expected '" << test[2] << "'\n"; - } else { - internal_assert(test[2].empty()) - << "Targets " << a.to_string() << " and " << b.to_string() << " were computed to have no gcd " - << "but " << test[2] << " was expected."; - } - } - - internal_assert(Target().vector_bits == 0) << "Default Target vector_bits not 0.\n"; - internal_assert(Target("arm-64-linux-sve2-vector_bits_512").vector_bits == 512) << "Vector bits not parsed correctly.\n"; - Target with_vector_bits(Target::Linux, Target::ARM, 64, Target::ProcessorGeneric, {Target::SVE}, 512); - internal_assert(with_vector_bits.vector_bits == 512) << "Vector bits not populated in constructor.\n"; - internal_assert(Target(with_vector_bits.to_string()).vector_bits == 512) << "Vector bits not round tripped properly.\n"; - - std::cout << "Target test passed\n"; -} - -} // namespace Internal - } // namespace Halide diff --git a/src/Target.h b/src/Target.h index c675ba74a7c0..30f998b769e6 100644 --- a/src/Target.h +++ b/src/Target.h @@ -236,6 +236,35 @@ struct Target { void set_features(const std::vector &features_to_set, bool value = true); + /** Set any feature flags that are implied by the flags currently set. For + * example, setting AVX2 implies AVX, so calling this on a target with the + * AVX2 feature will also set the AVX feature. The set of implications is a + * DAG, so this reaches a fixed point in a single pass. Call this before + * inspecting a target's features, so that (e.g.) a check for SSE41 + * succeeds on an AVX2 target. */ + void set_implied_features(); + + /** Unset any feature flags that are implied by other flags that remain + * set, producing the minimal set of feature flags that + * set_implied_features() would expand back to the same target. For + * example, on a target with both AVX2 and AVX set, this unsets AVX (since + * AVX2 implies it), but on a target with only AVX set it leaves AVX + * alone. Call this before emitting a target as a string, to get a compact + * canonical form. */ + void unset_implied_features(); + + /** Canonicalize the feature flags by calling set_implied_features() + * followed by unset_implied_features(). This fills in any missing implied + * flags and then removes any that are redundant, leaving the minimal set + * of flags that captures the target. */ + void normalize(); + + /** Return a copy of the target with set_implied_features() applied. */ + Target with_implied_features() const; + + /** Return a copy of the target with unset_implied_features() applied. */ + Target without_implied_features() const; + bool has_feature(Feature f) const; bool has_feature(halide_target_feature_t f) const { @@ -416,11 +445,6 @@ Target get_jit_target_from_environment(); * Target::FeatureEnd */ Target::Feature target_feature_for_device_api(DeviceAPI api); -namespace Internal { - -void target_test(); -} - } // namespace Halide #endif diff --git a/src/Tracing.cpp b/src/Tracing.cpp index 143b89472c34..f271869905af 100644 --- a/src/Tracing.cpp +++ b/src/Tracing.cpp @@ -73,9 +73,10 @@ class InjectTracing : public IRMutator { : env(e), trace_all_loads(t.has_feature(Target::TraceLoads)), trace_all_stores(t.has_feature(Target::TraceStores)), - // Set trace_all_realizations to true if either trace_loads or trace_stores is on too: - // They don't work without trace_all_realizations being on (and the errors are missing symbol mysterious nonsense). - trace_all_realizations(t.features_any_of({Target::TraceLoads, Target::TraceStores, Target::TraceRealizations})) { + // TraceLoads and TraceStores imply TraceRealizations (see + // set_implied_features), because tracing loads or stores doesn't work + // without the enclosing realization begin/end events. + trace_all_realizations(t.has_feature(Target::TraceRealizations)) { } private: diff --git a/test/correctness/target.cpp b/test/correctness/target.cpp index acd468e9a8c7..e35c686ea6d7 100644 --- a/test/correctness/target.cpp +++ b/test/correctness/target.cpp @@ -217,6 +217,140 @@ int main(int argc, char **argv) { return 1; } + // Every Target::Feature must have a name, and that name must map back to + // the same feature. + for (int i = 0; i < (int)Target::FeatureEnd; i++) { + Target::Feature f = (Target::Feature)i; + std::string name = Target::feature_to_name(f); + if (Target::feature_from_name(name) != f) { + printf("Feature %d does not round-trip through its name (%s)\n", i, name.c_str()); + return 1; + } + } + + // gcd(a, b) == c, computed via get_runtime_compatible_target. An empty c + // means the two targets have no compatible runtime. + struct GcdTest { + const char *a, *b, *c; + }; + const GcdTest gcd_tests[] = { + {"x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma"}, + {"x86-64-linux-sse41-fma-no_asserts-no_runtime", "x86-64-linux-sse41-fma", "x86-64-linux-sse41-fma"}, + {"x86-64-linux-avx2-sse41", "x86-64-linux-sse41-fma", "x86-64-linux-sse41"}, + {"x86-64-linux-avx2-sse41", "x86-32-linux-sse41-fma", ""}, + {"x86-64-linux-cuda", "x86-64-linux", "x86-64-linux-cuda"}, + {"x86-64-linux-cuda-cuda_capability_50", "x86-64-linux-cuda", "x86-64-linux-cuda"}, + {"x86-64-linux-cuda-cuda_capability_50", "x86-64-linux-cuda-cuda_capability_30", "x86-64-linux-cuda-cuda_capability_30"}, + {"x86-64-linux-vulkan", "x86-64-linux", "x86-64-linux-vulkan"}, + {"x86-64-linux-vulkan-vk_v13", "x86-64-linux-vulkan", "x86-64-linux-vulkan"}, + {"x86-64-linux-vulkan-vk_v13", "x86-64-linux-vulkan-vk_v10", "x86-64-linux-vulkan-vk_v10"}, + {"hexagon-32-qurt-hvx_v65", "hexagon-32-qurt-hvx_v62", "hexagon-32-qurt-hvx_v62"}, + {"hexagon-32-qurt-hvx_v62", "hexagon-32-qurt", "hexagon-32-qurt"}, + {"hexagon-32-qurt-hvx_v62-hvx", "hexagon-32-qurt", ""}, + {"hexagon-32-qurt-hvx_v62-hvx", "hexagon-32-qurt-hvx", "hexagon-32-qurt-hvx"}, + {"x86-64-windows-d3d12compute-hlsl_sm66", "x86-64-windows-d3d12compute", "x86-64-windows-d3d12compute"}, + {"x86-64-windows-d3d12compute-hlsl_sm66", "x86-64-windows-d3d12compute-hlsl_sm60", "x86-64-windows-d3d12compute-hlsl_sm60"}, + {"x86-64-windows-d3d12compute-hlsl_sm62", "x86-64-windows-d3d12compute-hlsl_sm62", "x86-64-windows-d3d12compute-hlsl_sm62"}, + {"x86-64-windows-d3d12compute-hlsl_sm69", "x86-64-windows-d3d12compute", "x86-64-windows-d3d12compute"}, + {"x86-64-windows-d3d12compute-hlsl_sm69", "x86-64-windows-d3d12compute-hlsl_sm60", "x86-64-windows-d3d12compute-hlsl_sm60"}, + }; + for (const auto &test : gcd_tests) { + Target result{}; + Target a{test.a}; + Target b{test.b}; + if (a.get_runtime_compatible_target(b, result)) { + if (std::string(test.c).empty() || result != Target{test.c}) { + printf("Targets %s and %s were computed to have gcd %s but expected '%s'\n", + a.to_string().c_str(), b.to_string().c_str(), result.to_string().c_str(), test.c); + return 1; + } + } else if (!std::string(test.c).empty()) { + printf("Targets %s and %s were computed to have no gcd but %s was expected\n", + a.to_string().c_str(), b.to_string().c_str(), test.c); + return 1; + } + } + + if (Target().vector_bits != 0) { + printf("Default Target vector_bits not 0.\n"); + return 1; + } + if (Target("arm-64-linux-sve2-vector_bits_512").vector_bits != 512) { + printf("Vector bits not parsed correctly.\n"); + return 1; + } + Target with_vector_bits(Target::Linux, Target::ARM, 64, Target::ProcessorGeneric, {Target::SVE}, 512); + if (with_vector_bits.vector_bits != 512) { + printf("Vector bits not populated in constructor.\n"); + return 1; + } + if (Target(with_vector_bits.to_string()).vector_bits != 512) { + printf("Vector bits not round tripped properly.\n"); + return 1; + } + + // Feature implications. Each entry is {input, set_implied_features result, + // normalize result}. + struct ImpliedTest { + const char *input; + const char *set_implied; + const char *normalized; + }; + const ImpliedTest implied_tests[] = { + // x86 AVX family + {"x86-64-linux-avx2", + "x86-64-linux-sse41-avx-avx2-f16c-fma", + "x86-64-linux-avx2"}, + {"x86-64-linux-avx512_skylake", + "x86-64-linux-sse41-avx-avx2-f16c-fma-avx512-avx512_skylake", + "x86-64-linux-avx512_skylake"}, + {"x86-64-linux-avx512_sapphirerapids", + "x86-64-linux-sse41-avx-avx2-f16c-fma-avxvnni-avx512-avx512_skylake-avx512_cannonlake-avx512_zen4-avx512_sapphirerapids", + "x86-64-linux-avx512_sapphirerapids"}, + // Redundantly-specified features collapse to the minimal form. + {"x86-64-linux-sse41-avx-avx2-f16c-fma", + "x86-64-linux-sse41-avx-avx2-f16c-fma", + "x86-64-linux-avx2"}, + // AVX10.1 implications depend on vector_bits. + {"x86-64-linux-avx10_1-vector_bits_512", + "x86-64-linux-sse41-avx-avx2-f16c-fma-avxvnni-avx512-avx512_skylake-avx512_cannonlake-avx512_zen4-avx512_sapphirerapids-avx10_1-vector_bits_512", + "x86-64-linux-avx10_1-vector_bits_512"}, + {"x86-64-linux-avx10_1", + "x86-64-linux-avx10_1", + "x86-64-linux-avx10_1"}, + // ARM v8.x cascade, and SVE/SVE2 cascading through arm_fp16. + {"arm-64-linux-armv84a", + "arm-64-linux-armv8a-armv81a-armv82a-armv83a-armv84a", + "arm-64-linux-armv84a"}, + {"arm-64-linux-sve2", + "arm-64-linux-armv8a-armv81a-armv82a-arm_fp16-arm_dot_prod-sve2", + "arm-64-linux-sve2"}, + // Apple silicon implies at least ARM v8.4a. + {"arm-64-osx", + "arm-64-osx-armv8a-armv81a-armv82a-armv83a-armv84a", + "arm-64-osx"}, + // Tracing loads/stores implies tracing realizations. + {"x86-64-linux-trace_loads", + "x86-64-linux-trace_loads-trace_realizations", + "x86-64-linux-trace_loads"}, + }; + for (const auto &test : implied_tests) { + Target set_result(test.input); + set_result.set_implied_features(); + if (set_result.get_features_bitset() != Target(test.set_implied).get_features_bitset()) { + printf("set_implied_features(%s) gave %s but expected %s\n", + test.input, set_result.to_string().c_str(), test.set_implied); + return 1; + } + Target norm_result(test.input); + norm_result.normalize(); + if (norm_result.get_features_bitset() != Target(test.normalized).get_features_bitset()) { + printf("normalize(%s) gave %s but expected %s\n", + test.input, norm_result.to_string().c_str(), test.normalized); + return 1; + } + } + printf("Success!\n"); return 0; } diff --git a/test/internal.cpp b/test/internal.cpp index f64bdfbca1a8..ddef5f95fa74 100644 --- a/test/internal.cpp +++ b/test/internal.cpp @@ -31,7 +31,6 @@ int main(int argc, const char **argv) { deinterleave_vector_test(); modulus_remainder_test(); cse_test(); - target_test(); cplusplus_mangle_test(); is_monotonic_test(); split_predicate_test(); From ee2cfcad8d6535cba35091a1654f8f6c2133f618 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 24 Jul 2026 15:09:56 -0700 Subject: [PATCH 2/2] Remove unused Target::normalize() It had no caller other than the target test. The test now calls set_implied_features() followed by unset_implied_features() directly, which is all normalize() did. Co-Authored-By: Claude Opus 4.8 --- src/Target.cpp | 5 ----- src/Target.h | 6 ------ test/correctness/target.cpp | 13 +++++++------ 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index 04e06e7370bc..020f07da68ea 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -1422,11 +1422,6 @@ void Target::unset_implied_features() { } } -void Target::normalize() { - set_implied_features(); - unset_implied_features(); -} - Target Target::with_implied_features() const { Target copy = *this; copy.set_implied_features(); diff --git a/src/Target.h b/src/Target.h index 30f998b769e6..aab31554cb37 100644 --- a/src/Target.h +++ b/src/Target.h @@ -253,12 +253,6 @@ struct Target { * canonical form. */ void unset_implied_features(); - /** Canonicalize the feature flags by calling set_implied_features() - * followed by unset_implied_features(). This fills in any missing implied - * flags and then removes any that are redundant, leaving the minimal set - * of flags that captures the target. */ - void normalize(); - /** Return a copy of the target with set_implied_features() applied. */ Target with_implied_features() const; diff --git a/test/correctness/target.cpp b/test/correctness/target.cpp index e35c686ea6d7..51acddda8207 100644 --- a/test/correctness/target.cpp +++ b/test/correctness/target.cpp @@ -290,11 +290,11 @@ int main(int argc, char **argv) { } // Feature implications. Each entry is {input, set_implied_features result, - // normalize result}. + // set-then-unset (minimal) result}. struct ImpliedTest { const char *input; const char *set_implied; - const char *normalized; + const char *minimal; }; const ImpliedTest implied_tests[] = { // x86 AVX family @@ -343,10 +343,11 @@ int main(int argc, char **argv) { return 1; } Target norm_result(test.input); - norm_result.normalize(); - if (norm_result.get_features_bitset() != Target(test.normalized).get_features_bitset()) { - printf("normalize(%s) gave %s but expected %s\n", - test.input, norm_result.to_string().c_str(), test.normalized); + norm_result.set_implied_features(); + norm_result.unset_implied_features(); + if (norm_result.get_features_bitset() != Target(test.minimal).get_features_bitset()) { + printf("set then unset implied features on %s gave %s but expected %s\n", + test.input, norm_result.to_string().c_str(), test.minimal); return 1; } }