Skip to content

Centralize Target feature-flag implication logic#9241

Open
abadams wants to merge 2 commits into
mainfrom
abadams/implied_features_2
Open

Centralize Target feature-flag implication logic#9241
abadams wants to merge 2 commits into
mainfrom
abadams/implied_features_2

Conversation

@abadams

@abadams abadams commented Jul 24, 2026

Copy link
Copy Markdown
Member

I asked Claude to clean up something that has been bugging me for a while:

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() (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.

abadams and others added 2 commits July 24, 2026 15:01
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@5476bf3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/CodeGen_X86.cpp 75.00% 1 Missing ⚠️
src/StmtToHTML.cpp 50.00% 1 Missing ⚠️
src/Target.cpp 98.21% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9241   +/-   ##
=======================================
  Coverage        ?   70.33%           
=======================================
  Files           ?      255           
  Lines           ?    78850           
  Branches        ?    18842           
=======================================
  Hits            ?    55462           
  Misses          ?    17841           
  Partials        ?     5547           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexreinking
alexreinking self-requested a review July 25, 2026 14:48
@alexreinking

Copy link
Copy Markdown
Member

This is a good change in spirit, but we could go further. The key idea is to lean into the algebra of targets.

I think a better design is to keep targets always implied-feature-complete internally (construction and parsing close the feature set) and make to_string always print the minimal form. Feature-implication is a partial order, so a target's feature set is either the down-closed set or the antichain of its maximal elements, and both name the same element of the lattice. Complete-in-memory / minimal-on-print picks the right canonicalization for each side. As it stands, this API makes canonicalization the client's responsibility, so it can happen at the wrong time (or not at all!).

For example:

  • without_implied_features is only ever called immediately before printing. If you forget to do that, the client sees a mess of a target string.
  • Pipeline::compile_to_module has to say old_module.target() == target.with_implied_features(), because operator== compares representations rather than targets.
  • In this implementation, the desirable property Target(m.target().to_string()) == m.target() doesn't hold.

If we do adopt the implied-feature-complete internal representation, we can simplify other code. get_runtime_compatible_target computes the meet in this lattice, and a large chunk of it exists only to work around the non-canonical representation: the ARM v8.x, CUDA capability, Vulkan and HLSL SM chains all sit in union_features under the comment "because targets only record the highest, we have to put their union in the result and then take a lower bound", followed by ~80 lines of lower-bound fixups. If every target were already closed, those become ordinary intersection features and the fixups delete themselves. As it stands the ARM v8.x chain is now encoded twice: in implied_feature_pairs() and in that union list.

So I don't think with_implied_features/without_implied_features belong in the API at all. set_implied_features should be a private invariant-restoring helper. A debug printer that dumps the full feature set seems reasonable, but it's clearly a testing-oriented API.

One gotcha is that arch, os and vector_bits are public mutable fields and the AVX10.1 and Apple-silicon implications depend on them, so switching to "always complete" would require making those fields private with setters that maintain the invariant. I don't think Target should have any unchecked mutable state anyway; every mutation should go through something that can restore invariants.

@abadams

abadams commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

Target(m.target().to_string()) == m.target() should hold. to_string and parsing both ignore implications. If non-canonical states are representable, then they need to be printable and parseable.

The alternative you propose, where non-canonical states aren't even representable, was actually something I proposed at an earlier dev meeting, but it wasn't popular at the time. We can discuss more in person, but I don't think it's a reason to block this PR - this is better than what we have now and it's a step in right direction.

@alexreinking

Copy link
Copy Markdown
Member

Target(m.target().to_string()) == m.target() should hold. to_string and parsing both ignore implications.

Oops, I got the issue confused in my notes; I was using m to denote a Module. The property that changes is that the target passed to lowering is not necessary the one attached to the resulting Module. It is canonicalized at src/Lower.cpp:622-623. So:

t != f.compile_to_module({}, "f", t).target()

Meanwhile, this holds:

t.with_implied_features() == f.compile_to_module({}, "f", t).target()

If non-canonical states are representable, then they need to be printable and parseable.

I agree.

The alternative you propose, where non-canonical states aren't even representable, was actually something I proposed at an earlier dev meeting, but it wasn't popular at the time.

Do you remember which meeting that was, or what objections were raised? I recall raising the same issue when I implemented get_runtime_compatible_target, and I also recall it being rejected, but not the rationale.

We can discuss more in person, but I don't think it's a reason to block this PR - this is better than what we have now and it's a step in right direction.

To be clear, I wasn’t blocking the PR; I was giving design feedback.

@alexreinking

Copy link
Copy Markdown
Member

Opened #9242 to track the fuzz_solve failure here. Restarted CI.

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.

2 participants