Conversation
zzylol
added a commit
that referenced
this pull request
Aug 24, 2026
- docs/asap_aware_mapping.md: rewrite the "Applicability reporting"
section for clarity, per review comments:
- define "site" explicitly (a TargetSubDAG — one place in the
pre-ASAP tree a ReplacementStrategy could replace) the first time
it's used, instead of assuming it's already known.
- split the original run-on sentence spanning "for every site... the
trivial, no-op realization for it" into short, direct sentences.
- define "trivial, no-op realization" explicitly, with a concrete
example (the lone exact accumulator / lone pass-through a site
would get regardless of any strategy).
- lead with an explicit answer to "how is applicability decided":
candidate count at a site, not a separate rule or a cost model —
then back it with three concrete bullet cases (more than one
candidate; one candidate that's a real alternative; only the
default, so no finding).
- applicability.rs: rename OptimizationKind::SharedSubexpressionReuse
to CommonSubexpressionReuse — "common subexpression" is the
well-known compiler/optimizer term (matches this repo's own
share_common_subtrees/CseCandidate naming), "shared subexpression"
isn't standard vocabulary. Renamed the doc comment, the variant, all
match sites, and the doc's own reference to it; SharedSubtreeStrategy
(a distinct type name in replacement.rs) is untouched.
Verified: cargo build --workspace --all-targets, cargo test
--workspace (0 failures), cargo fmt --all -- --check, cargo clippy
--workspace --all-targets --all-features -- -D warnings — all clean.
zzylol
force-pushed
the
feat/cascades-search-252
branch
from
August 24, 2026 14:03
9d22b7e to
7b14b4b
Compare
…'s current tip #263 (feat/cascades-search-252) was just rewritten (force-pushed) as part of syncing every #33 sub-issue PR to #259's current tip. This branch, stacked on #263, was still built on #263's old (pre-rewrite) history, which made the two branches diverge and GitHub start reporting this PR CONFLICTING against its base. Rebuilt fresh: reset onto #263's current tip, then reapplied applicability.rs (already updated per this PR's own review-comment fixes: "site" -> TargetSubDAG throughout, stale boundary:: doc links fixed) verbatim, since it depends only on crate::search/crate::replacement, both stable and unchanged in shape across the rewrite. Wired it into lib.rs (pub mod applicability;, its Status bullet, and its exports), and picked up two small search.rs doc improvements this PR had made (replacing "a hypothetical applicability::..." with real references, now that this module exists) that were otherwise lost. The design doc's old "## Applicability reporting" section this PR originally added lived at docs/asap_aware_mapping.md, a path deleted by the docs reorg (#265) that landed on main in the meantime; the design doc's current location (docs/design_docs/asap_aware_mapping.md) already carries its own "# Applicability Reporting" section, written independently and already consistent with this PR's own terminology fix (uses "Target Sub-DAG" throughout, no "site"). Left it untouched rather than reintroducing now-orphaned content. Verified: cargo build --workspace --all-targets, cargo test --workspace (0 failures, including all 11 applicability:: tests), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings — all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
feat/applicability-view-257
branch
from
August 24, 2026 14:30
0dcb8f5 to
1566419
Compare
zzylol
added a commit
that referenced
this pull request
Aug 24, 2026
…nSpace (#257, part of #33) Absorbs PR #264 (feat/applicability-view-257) into this branch, re-derived against the current tip rather than merged/rebased/cherry-picked (#264's base predates the SketchFamilyStrategy->SketchAlgorithmStrategy rename, the search.rs->replacement.rs merge, and the "site"->"target"/TargetSubDAG rename). Replaces the deleted PR #247 rule-based traversal with a pure view over replacement::PlanSpace: a TargetSubDAG is applicability-worthy exactly when its candidate list contains something beyond the trivial, no-op realization. - OptimizationKind::SketchApproximation — the candidate list contains a Replacement::Summary that realizes SummaryFamilyType::Sketch(..). - OptimizationKind::CommonSubexpressionReuse — consumer_count >= 2 and the candidate list contains SharedSubtreeStrategy's "build once and share" candidate. Each finding's `reason` is the matching candidate's own ReplacementSubDAG::rationale verbatim — no new prose. No new applicability-specific extension-point trait: a new finding needs a new impl ReplacementStrategy wired into default_strategies/default_strategies_with regardless, since that's the only way a candidate reaches PlanSpace at all. Kept as its own module (crates/asap-aware-mapping/src/applicability.rs, `pub mod applicability;` in lib.rs) rather than flattened into replacement.rs: unlike implementation.rs/search.rs, this is a reporting/presentation transformation (internal PlanSpace -> external ApplicabilityFinding shape for a downstream DAG-visualization consumer), a genuinely different kind of concern from generating or ranking candidates. Also fixes now-stale cross-references in replacement.rs that pointed at the deleted PR #247 ApplicabilityRule/SharedSubexpressionRule/register_site traversal, and a stale reference in the developer guide (section 4.2) to the same deleted trait. Adds an "Applicability reporting" section (§19) and an extension-map row to docs/developer_docs/ASAP-aware-mapping-developer-guide.md; docs/design_docs/asap_aware_mapping.md already carries a conceptual "Applicability Reporting" section and, matching this session's convention of keeping that document implementation-agnostic, is left as-is. cargo build --workspace --all-targets, cargo test --workspace, cargo fmt --all -- --check, and cargo clippy --workspace --all-targets --all-features -- -D warnings all pass clean. PR #264 (feat/applicability-view-257) and PR #263 (feat/cascades-search-252) are both now superseded by this branch; flagged for the user to close. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Superseded — its content (applicability.rs as a view over PlanSpace) was folded directly into #259 (feat/replacement-strategy-251) rather than staying a separate stacked PR. |
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.
Closes #257. Part of #33. Stacked on #263 (issue #252) and #259 (issue #251) — do not merge before those two.
What changed vs. the old rule-based traversal
crates/asap-aware-mapping/src/applicability.rs(PR #247, unmerged) re-derived "is optimization X applicable" as its own yes/no fact, walking the tree a second time per rule (SketchApplicabilityRulewrappingboundary::implementation_for_with,SharedSubexpressionRulewrappingshare_common_subtrees). Now thatsearch::search_workload(#252, over theReplacementStrategyextension point from #251) already computes, per site, every candidate replacement any registered strategy proposes, this module is rebuilt as a pure view over thatPlanSpace:OptimizationKind::SketchApproximation— the site has at least oneReplacement::Summarycandidate that actually realizes a sketch family (not just the exact/pass-through candidate the boundary would commit to anyway).OptimizationKind::SharedSubexpressionReuse— the site hasconsumer_count >= 2and its candidate list containsSharedSubtreeStrategy's "build once and share" candidate.reasonis literally the matching candidate's ownrationalestring — no new prose invented to re-explain why a candidate is valid.ApplicabilityFinding/OptimizationKind/find_applicable_optimizations/find_applicable_optimizations_withkeep their exact public shape and contract (same fields, same two variants, same#[non_exhaustive]discipline).find_applicable_optimizations_withnow takes a&[Box<dyn ReplacementStrategy>](mirroringsearch_workload_with) instead of a rule list — the natural consequence of strategies replacing rules as the underlying data source.SketchApplicabilityRule,SharedSubexpressionRule,collect_sketch_findings,register_site,walk_rc_children,for_each_operator_child, and theApplicabilityRuletrait itself.ApplicabilityRuleis redundant withReplacementStrategyonce findings are a view overPlanSpace— a new optimization needs a newimpl ReplacementStrategywired intosearch::default_strategies()regardless (that's the only way its candidates reach thePlanSpacethis module reads), so a second, applicability-specific extension point would just duplicate that one. Documented in the module doc, mirroringreplacement.rs's own "why this exists alongside boundary and bind" style.collect_locations, whose only job is turning "thisRc" into human-readable breadcrumb text (root "dash_a" > lhs) forApplicabilityFinding::location—PlanSpace/MemoGroupdeliberately track onlyRcpointer identity, not caller-facing prose. It makes no applicability decision of its own; it runs the same regardless of what any strategy found.lib.rs's module-level doc (added theapplicabilitybullet, and asearchbullet that had gone missing from an earlier PR) and added a new "Applicability reporting" section todocs/asap_aware_mapping.md. Fixed a handful of now-stale cross-references inreplacement.rs/search.rsthat pointed at the deleted PR feat(asap-aware-mapping): optimization-applicability rule framework #247 traversal.Old guarantees re-verified against the new implementation
The two properties the old traversal's docs specifically called out are both still upheld, by mechanisms now living in
search.rsrather than here:search::discover_sitesonly recurses into a node's children the first time that node'sRcis seen (exactlyregister_site's old discipline) — a node nested under an already-discovered shared ancestor only becomes its own site if something outside that ancestor also references it.MemoGroups are keyed byRcpointer identity, so there is exactly one group (hence at most one finding) per distinctRc, regardless of how many paths reach it.a_shared_sketchable_aggregate_is_reported_only_once(pinning guarantee 2) passes unchanged. Addeda_deeply_shared_subtree_under_different_parents_is_reported_onceas a further regression test for guarantee 1 (mirrorssearch::tests::nested_shared_subtree_below_an_unshared_parent_is_still_discovered).All other existing tests (positive/negative fixtures for both sketch and reuse findings, custom-strategy plumbing) were preserved with only mechanical updates (e.g.
SketchApplicabilityRule::new→SketchFamilyStrategy::newpassed as a strategy).Checks
cargo build --workspace --all-targets,cargo test --workspace,cargo fmt --all -- --check, andcargo clippy --workspace --all-targets --all-features -- -D warningsall pass clean.🤖 Generated with Claude Code