[2/3] fix: prune spills by reachability, not dominance - #1250
Conversation
1e4894b to
91dcdca
Compare
5e58631 to
c3e2aa6
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
This will need to be rebased unfortunately, since #1247 was merged and this seemingly was branched off that PR
There are a couple of issues with the way reachability is implemented here. Ultimately, what we really want is a proper reachability analysis that works in concert with SCCP/DCE, so we can elide spills even when there are reloads that are reachable from the view of the CFG, but only via paths that aren't actually executable (e.g. spill(); if false { let v = reload(); foo(v); } else { bar(); } where the reload can't possibly ever be executed).
That said, a simpler form that we can implement as a heuristic could still be useful, at least as a short-term workaround for our lack of reachability analysis - but I think we need to adjust the implementation here a bit, as noted in my comments.
| /// This query is exported from the crate root chiefly so the behavioral tests in | ||
| /// `midenc-dialect-hir` (which have the parser and control-flow dialects needed to build | ||
| /// fixtures) can exercise it directly; the in-crate consumer is spill pruning. | ||
| pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { |
There was a problem hiding this comment.
Let's make this a static function hanging off Operation, since it is a very general query that could be useful in other contexts.
|
|
||
| /// Returns true if some control-flow path from `from` may reach `to`. | ||
| /// | ||
| /// The result may be conservatively `true` when reachability cannot be reasoned about precisely; |
There was a problem hiding this comment.
The semantics of this query are incorrect:
- The name of this function is
op_reaches, but that is not in fact the question it answers. Instead, it is more likeop_might_reach. A more interesting question to ask (and answer), at least for the spills elision optimization, is something more likeop_cannot_reach, i.e. you want to know whether you can safely remove a spill because none of its corresponding reloads are reachable. - Reachability cannot be statically proven as claimed by this function. The only way that property can be usefully proven in general is with a real reachability analysis (which this is not), performed in conjunction with the SCCP/DCE analyses (so you can determine what control flow paths are actually reachable at runtime). The cases that are statically provable with this query as written, are the least interesting ones (i.e. the two operations have no conditional control flow between them).
- This function should return a more precise answer, so that the action taken when the query can't prove reachability is left up to the caller. Something like:
enum Reachability {
/// Provably unreachable, i.e. no control flow path exists between `a` and `b`
Impossible,
/// Provably reachable, i.e. there is at least one control flow path guaranteed to reach from `a` to `b`
Guaranteed,
/// Reachability is not proven, but there is at least one control flow path that reaches from `a` to `b`, but full reachability analysis is required to prove whether the path(s) are truly executable
Maybe,
/// Cannot be determined without global reachability analysis, because the two ops are in different functions
MaybeInterprocedurally,
/// Cannot be determined because control flow between the two ops is not well-defined (i.e. the both belong to a graph-like region, or their common ancestor region is graph-like)
Indeterminate,
}The spills transform would treat both MaybeInterprocedurally and Indeterminate as errors, because they represent invalid states for the IR to be in (a reload of a spill must necessarily belong to the same function; and spills/reloads aren't valid in graph-like regions at all). Other users of this function might act on those results differently.
| // Operations in different isolation scopes (e.g. two functions) share no control flow to | ||
| // walk, so the query cannot be answered; report the conservative `true`. | ||
| if isolation_scope(from) != isolation_scope(to) { | ||
| return true; |
There was a problem hiding this comment.
In the case of spills/reloads, this is definitely a hard error kind of situation, or "provably unreachable" - because it is never the case that a spill and a reload in different isolation scopes are related to each other in any way (by definition, a reload can only reference a spill in the same isolation scope).
| // nested in a sub-region (e.g. structured control flow) is represented by its ancestor | ||
| // operation in the common region. | ||
| let Some(common_region) = Region::find_common_ancestor(&[from, to]) else { | ||
| // Operations without a common ancestor region cannot be reasoned about. |
There was a problem hiding this comment.
IMO, we absolutely can return false here. A lack of a common ancestor (at least in the context of spills) effectively makes it impossible for control flow to reach b from a, or a to b (since if either was possible, there would necessarily be a common ancestor, i.e. a region/block that is an ancestor of both).
Let me know if I'm missing an obvious case here - but it isn't at all clear to me why we'd be conservative in this case.
I should also note, that at least for spills, it is just a straight up error for a reload/spill pair to lack a common ancestor - all reloads of a value must be strictly dominated by spills of that value. An individual spill might not dominate a given reload of that spilled value (e.g. a reload at a program point where control flow joins may have likely spilled different values along the paths taken to the join), but there cannot be a path to the reload that doesn't go through at least one spill of the reloaded value.
| return true; | ||
| } | ||
|
|
||
| // Ancestors that are themselves isolated from above (e.g. two functions in one module) are |
There was a problem hiding this comment.
This comment is incorrect. The IsolatedFromAbove trait means that regions of that op cannot reference SSA values defined "above" that op (including the scope that the op itself appears in). It has nothing to do whatsoever with symbols, and does not imply anything about the region that contains it (i.e. an IsolatedFromAbove op can appear in either SSA CFG regions or graph-like regions).
So if the ancestor ops are isolated from above, then by definition spills/reloads cannot be shared between them (because no SSA value that was spilled above them could have been referenced within them). This function doesn't deal with spills/reloads directly, only control flow reachability though.
In any case, since IsolatedFromAbove implies nothing that would affect reachability from one op to another, I don't understand the purpose of this (and I suspect it is actually problematic to have this here)
|
|
||
| /// Returns the nearest proper ancestor of `op` that is isolated from above, i.e. the operation | ||
| /// whose single execution scopes a reachability query involving `op`. | ||
| fn isolation_scope(op: OperationRef) -> Option<OperationRef> { |
There was a problem hiding this comment.
This doesn't have any bearing on reachability between ops - at best you could search upwards until you find the nearest ancestor op whose parent region is a graph-like region (since reasoning about reachability beyond that point is impossible/not meaningful).
For spills, we'd necessarily be looking for the nearest Function - but since Function is always defined in a graph-like region, it trivially meets the above criteria.
c3e2aa6 to
8eefc18
Compare
|
Thank you for a thorough review! I implemented everything in the last commit. Please do another round. |
|
@greenhat Can you rebase this on |
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
This is just about ready I think, but Codex found some issues that escaped my first pass over the changes, and I agree with the conclusions. See the comments for details. NOTE: I did the review against a local merge of the latest next branch with your PR on top - so bear that in mind, but I don't think it changes anything as far as this review goes
| // (and control entering it can reach the nested position), or they sit in different | ||
| // sub-regions of that op, where transfer between the regions depends on the op's | ||
| // semantics (e.g. it can happen across loop iterations). | ||
| if from_ancestor == to_ancestor { |
There was a problem hiding this comment.
[P2] Consult the sibling-region graph
Operations in mutually exclusive scf.if arms normalize to the same owner, but no child-to-sibling path exists unless an enclosing construct can re-execute the if. Returning Maybe here prevents spill pruning from removing provably dead stores. Consult RegionBranchOpInterface and outer re-entry before deciding.
| // Within one block an earlier operation always flows into a later one; this is only a | ||
| // guarantee when neither position was normalized, since entering a sub-region of an | ||
| // ancestor op is generally conditional on that op's semantics. | ||
| if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { |
There was a problem hiding this comment.
[P2] Do not guarantee passage through intervening ops
Earlier placement in the same block does not ensure control reaches the later operation: an intervening region operation may loop indefinitely, return from the function, or abort. Return Maybe unless every intervening operation is proven to return to its parent on all paths.
| // op. This must precede the scope comparison, which would otherwise misclassify | ||
| // enclosure by an op residing in a graph-like region (e.g. a function op and an op in | ||
| // its body) as interprocedural. | ||
| if from.borrow().is_proper_ancestor_of(&to.borrow()) |
There was a problem hiding this comment.
[P2] Preserve graph-region indeterminacy
This ancestry shortcut runs before region-kind checks, so a module and a function nested in its graph-like body are reported as Maybe even though that region defines no control-flow order. Inspect the crossed region chain and return Indeterminate when enclosure crosses a graph boundary.
| if !reload_used { | ||
| continue; | ||
| } | ||
| match Operation::reachability(operation, reload_op) { |
There was a problem hiding this comment.
[P2] Avoid repeated whole-CFG walks
Every spill scans every reload, and each matching live pair starts a fresh structural reachability traversal here. This is O(S × R × (B + E)) and can become cubic. Index live reloads by ValueRef and cache or reverse-compute block reachability per value.
The spills analysis deliberately places spills per path, e.g. one along each arm of a join, so a reload placed after the join is covered by the set of arm spills, none of which individually dominates it. The spill pruning in rewrite_spill_pseudo_instructions kept a spill only when it dominated a live reload, so it erased every covering spill of such a reload while the reload itself survived, and reload materialization then panicked looking up a procedure local that was never allocated. Had the value also owned a second, dominated reload, one spill would survive and uncovered paths would silently read a stale local instead. Replace the dominance predicate with reachability: a spill is elided only when no control-flow path from it can reach a live reload of its value. Operations are first normalized to their ancestors in the innermost common region, so spills nested in structured control flow (which have no CFG successors) are still handled, and every case that cannot be reasoned about conservatively keeps the spill. This preserves dead edge-spill elimination, since a consistency spill whose only reload sits in a sibling arm reaches nothing and is still erased, and it makes the previously unpruned single-block path use the same criterion. The now-unused dominfo parameter is dropped, and convert_reload_to_load reports a descriptive error instead of panicking if a live reload ever loses all covering spills again. Add an end-to-end regression test in which both arms of a diamond spill the same value and the sole reload lands at the join, plus unit tests pinning spill_reaches_reload (now exported) across diamond, loop back-edge, and nested scf.if shapes. Existing spill fixtures are unchanged.
The spill-pruning regression tests built their fixtures with IR builders and pinned results via expect-files and line-count assertions, which made the CFG shapes hard to read and coupled the tests to builder APIs. Rewrite the spill_reaches_reload unit tests to parse textual IR fixtures (via parse_function_fixpoint, locating operations positionally), and move the join-covered-reload regression to tests/lit/hir-opt, where hir-opt runs a transform-spills pass pipeline over the parsed module and filecheck pins the materialized output: one store_local per arm to the same procedure local, and a single load_local at the join. Register TransformSpills in the global pass registry so pass pipelines can refer to it by name; previously it was not invocable from hir-opt at all. Both layers independently guard the fix: against dominance-based pruning the lit test fails with the live-reload-without-spill diagnostic, and each reachability unit test fails on the assertion encoding the corresponding dominance blind spot (arm-to-join, back-edge-to-header, nested-region-to-parent-successor).
…pruning spill_reaches_reload answered false for a reload positioned before a spill in the same region of a loop op (e.g. the before region of an scf.while): the region's block ends in a region terminator with no block successors, so the CFG successor walk finds nothing, while the loop's back edge lives in the region graph of the owning op. That violates the predicate's contract that false means provably unreachable, since the region re-executes and control genuinely flows from the spill back around to the reload. Today the erased spill is always redundant, as every spill of a value stores the same SSA value to one per-value local and the analysis keeps loop-entry paths covered, but any consumer of the exported predicate, or a future change to spill slot allocation, would inherit a stale-read miscompile. When the forward walk fails, climb the region ancestry from the common region and report reachable if any enclosing region is repetitive, stopping at isolated-from-above boundaries (fresh locals per invocation) and conservatively keeping spills under ops with unknown region semantics. Document the directional contract on the predicate: true may be conservative, false must be proof. Pin the repaired behavior with a unit test covering re-entry of an scf.while region, forward reachability within its block, and no-reach-back-in from after the loop.
The ReloadLike contract still said a reload requires a dominating SpillLike op, which is exactly the invariant the reachability-based pruning removed: a reload after a join is covered by a set of per-path spills, none of which individually dominates it. Restate the contract in path-coverage terms so the old dominance reasoning is not reintroduced from the docs, and clarify in the transform_spills overview that dominance governs only the SSA use rewrite while spill materialization is decided by reachability.
The predicate is plain operation-to-operation reachability: its body never touches SpillLike or ReloadLike, and the unit tests exercise it on arbitrary arith ops, which read oddly through a spill-specific name. Rename it to op_reaches, document the generic contract on the public item (conservative true, false is a proof, scoped to a single execution of the innermost isolated-from-above ancestor), and keep the spill-pruning rationale with the pruning itself in the rewrite_spill_pseudo_instructions docs.
…test The gap between the then-arm's call and the else-arm label had no CHECK-NOT, so an extra unpruned store after the first call would have passed unnoticed. Add the symmetric CHECK-NOT so the test pins exactly one store per arm on both arms.
…tion scope op_reaches documents false as a proof of unreachability, but a query crossing an isolation boundary broke that contract: for operations in two sibling functions, normalization silently climbed out into the module body and the answer degenerated to their textual order there, returning true one way and false the other. The false direction is meaningless for such queries, since the module body is a graph region whose block order is not control-flow order, and cross-function control transfer is not a walkable path. Guard the query up front by comparing the operations' innermost isolated-from-above ancestors and answering the conservative true when they differ, treat normalized ancestors that are themselves isolated (sibling functions in one module) the same way, and mirror the dominance precedent for same-block queries in regions without SSA dominance. Document the precondition, mark the unreachable find_ancestor_op fallback as defensive, and reword the same-ancestor comment to cover the enclosing-op case where true is exact rather than conservative. Pin the guards with a unit test querying across sibling functions and their module, in both directions. The only current caller passes two positions of one function body, so no miscompile was reachable; this hardens the exported contract.
…rror The missing-spill branch in convert_reload_to_load can only be reached through a compiler bug (pruning erased every covering spill), yet the message read like a statement about the input program and carried no location. Prefix it as an internal error and name the function being transformed so a future report is self-locating.
The load_local check was only required to appear somewhere after the else-arm's call, so a regression that sank the reload into an arm would still have passed. Capture the join label from both arm terminators, which also pins that the arms branch to the same join, and require the reload to appear inside that block as its first spill-slot access.
…able in op_reaches op_reaches recognized only region-graph cycles as a re-entry mechanism after the forward block walk failed: for two ops inside an scf.if region whose host block lies on a CFG cycle, the region is not repetitive in the op's own region graph, so the query answered false even though every loop iteration re-enters the region. That breaks the contract that false proves unreachability, and would let pruning erase a spill that a reload on the next iteration reads. The shape cannot be produced by the current pass schedule, which runs the transform on pure CFGs before control flow is lifted and on single-block bodies after, but nothing enforces that precondition and op_reaches is an exported API. Check both re-entry mechanisms at every level of the region ancestry walk: a repetitive enclosing region, or an enclosing op whose block can reach itself through block successors. Extract the forward walk as block_leads_to and the ancestry walk as region_can_re_execute, so the tail of op_reaches reads as the two-sentence summary its doc gives. Pin the repaired behavior with a regression test placing an scf.if on a CFG loop.
Four of the six tests exercising op_reaches carried a spill_reachability_ prefix from before the predicate was renamed, while the newer two already used op_reaches_. Rename the four tests and their fixtures so all six name the unit under test and group together in test listings.
… pruning The pruning loop re-derived the spilled value from the spill op's current operand through a SpillLike cast, then matched it against ReloadInfo::value. SpillInfo::value is the same analysis-domain identity, so the derivation was an indirection with a hidden assumption baked in: SSA reconstruction exempts only reload operands from rewriting, so a spill placed after a reload of the same value on one path would have its operand redirected, match no reloads, and be erased despite covering them. The analysis does not produce that shape today, but the pruning decision should not depend on it. Match the analysis's own value on both sides and drop the derivation.
Three clarifications that protect invariants a future refactor could otherwise break: block_leads_to is not reflexive (a self-query is a cycle test, which region_can_re_execute relies on), the two isolation guards in op_reaches cover disjoint cases and neither subsumes the other, and op_reaches is exported chiefly for the behavioral tests in midenc-dialect-hir rather than as a stable public surface.
…perand Spill pruning pairs spills with reloads through the analysis's value bookkeeping, but materialization still keyed the per-value procedure local by the spill op's current operand while reload lookup used the reload's operand. SSA reconstruction exempts only reload operands from use rewriting, so a kept spill whose operand was redirected (e.g. to a preceding reload's result or an inserted phi) allocated and wrote a second local that no reload reads; had every spill of a value been rewritten, covered reloads would have failed with the internal error on IR the analysis handled correctly. Thread the analysis value through TransformSpillsInterface and key the locals map by it on both sides, while the store keeps writing the op's current operand, which is the correctly split live range at that point and always carries the same runtime value. This makes the internal error unreachable for covered reloads and drops the reliance on reload operands being exempt from rewriting. Pin the behavior with a synthesized spill-reload-spill-reload chain over one value, where the rewrite phase redirects the second spill's operand to the first reload's result: the transform must materialize a single shared local, storing the reload result into it.
Implements the review feedback on the reachability query used by spill pruning. The boolean op_reaches helper over-claimed its name and folded every unanswerable case into a conservative true, and it reasoned about query boundaries via IsolatedFromAbove, which governs SSA value visibility and implies nothing about control flow. Move the query into midenc-hir as Operation::reachability, a general positional query returning a Reachability classification: Impossible and Guaranteed are proofs, Maybe covers paths whose executability only a proper reachability analysis (in concert with SCCP/DCE) could decide, and MaybeInterprocedurally/Indeterminate report queries that leave a single function's control flow or land in a graph-like region, leaving their interpretation to the caller. All boundary reasoning is now based on region kinds: the intra-procedural scope is bounded by the nearest ancestor residing in a graph-like region (in practice the enclosing function), the same boundary terminates the re-execution walk, and a graph-like common ancestor region is Indeterminate. A missing common ancestor region is now correctly Impossible rather than conservatively kept, since any control-flow path would itself lie in a common region. Spill pruning keeps a spill for Guaranteed/Maybe reloads, treats Impossible as not covering, and fails with an internal error on MaybeInterprocedurally/Indeterminate, both of which are invalid IR for a spill/reload pair. Also replaces the intra-doc link to the private rewrite function with plain code formatting, which previously broke cargo doc under deny(warnings).
…sifications Addresses the second review round on the reachability query. Sibling sub-regions of one region-branch op now consult the op's own region graph instead of a blanket Maybe: the arms of an if outside any loop are provably unreachable from each other, which lets spill pruning remove dead sibling-arm stores, while the before/after regions of a while remain mutually reachable, and a negative region-graph answer still falls back to whether the owner op itself can execute again (an enclosing repetitive region or a CFG cycle through its block), so arms of an if that a loop re-enters stay reachable. Guaranteed is now claimed only for adjacent positions in one block: an intervening operation may loop indefinitely, return from the function, or abort, so earlier placement alone proves nothing beyond Maybe. Enclosure classifies the chain of regions crossed between the two operations: crossing a graph-like region (e.g. a module enclosing a function) defines no control-flow order and is Indeterminate, while enclosure within a function remains Maybe.
The pruning loop scanned every reload for every spill and ran a fresh block-reachability walk per matching live pair, O(spills x reloads x CFG) in the worst case. Index the live reloads by spilled value once, snapshotting reload liveness before any erasures (which errs toward keeping a spill whose reload only dies as part of the erasure cascade), so each spill only considers reloads it can possibly cover. Add ReachabilityCache to midenc-hir: a lazily-populated forward-closure cache shared across Operation::reachability_cached queries, so the first query from a block computes its reachable set once and every later query from that block is a set lookup. Pruning holds one cache for the whole rewrite, which stays valid because pruning erases operations, never blocks.
8eefc18 to
f8571df
Compare
|
@bitwalker Thank you! Done in 76c18ec and f8571df. Please do another round. |
Classify forward same-block operation pairs as maybe reachable because adjacency and ordering do not prove that the source or intervening operations fall through.
Prove ancestor entry and descendant exit paths through callable CFGs and concrete region-branch terminators. Preserve conservative maybe results for unmodeled owners, terminators, and nested control flow.
2 participants
Close #1249
The spill transform erased any spill that did not dominate a live reload of its value. The spills analysis places spills per path, so a reload after a join is covered by a set of per-arm spills, none of which dominates it: all were erased while the reload survived, panicking reload materialization (or, with partial coverage, reading a stale local — a silent miscompile).
Prune by reachability: a spill is elided only if no control-flow path from it can reach a live reload of its value, after normalizing nested operations to their ancestors in the innermost common region; unanalyzable cases conservatively keep the spill. Dead edge-spills are still erased.