diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index ba79b8b..ab6da95 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -7,24 +7,36 @@ on: env: CARGO_TERM_COLOR: always +permissions: + contents: read + jobs: publish: name: Publish runs-on: ubuntu-latest + permissions: + contents: read + id-token: write # required to mint the crates.io OIDC token steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - # Guard against publishing a version that doesn't match the release tag. - - name: Verify tag matches Cargo.toml version + with: + fetch-depth: 0 # merge-base needs real history + + # A GitHub release can be cut from any commit, including one that never + # landed on main. Publishing is restricted to release tags that are + # actually contained in main. + - name: Refuse releases not contained in main run: | - crate_version="$(cargo metadata --no-deps --format-version 1 \ - | grep -o '"version":"[^"]*"' | head -1 | cut -d'"' -f4)" - tag_version="${GITHUB_REF_NAME#v}" - if [ "$crate_version" != "$tag_version" ]; then - echo "::error::tag $tag_version != Cargo.toml $crate_version" - exit 1 - fi - - run: cargo test --all-features + git fetch --no-tags origin +refs/heads/main:refs/remotes/origin/main + git merge-base --is-ancestor "$GITHUB_SHA" refs/remotes/origin/main \ + || { echo "::error::release commit $GITHUB_SHA is not contained in main"; exit 1; } + + - uses: dtolnay/rust-toolchain@stable + - run: cargo test + + - uses: rust-lang/crates-io-auth-action@v1 + id: auth + - run: cargo publish env: - CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }} + CARGO_REGISTRY_TOKEN: ${{ steps.auth.outputs.token }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e58f42..ce4f84f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,143 @@ All notable changes to this crate are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.0] - 2026-08-04 + +Correctness of the SSA rebuild and the phi transforms. On a 125 MB x86-64 +reference binary (50,000 functions), pass rollbacks went from 6,094 to **0** and +verifier-reported undefined uses from ~28,960 to **0**; no function is now +floored by normalization. Output is 6% leaner (10,089,320 to 9,490,820 lowered +rows) while landing 4% more rewrites (94,257 to 98,467), because rejected work is +no longer discarded wholesale. + +Every defect below shares a shape: a value substitution or a definition record +that was *stated* rather than *established* — a chain composed without resolving +to a surviving target, a label asserting a definition that did not exist, or a +repair applied on one path and not its sibling. + +### Changed + +- **A pass group no longer trusts a pass's own "changed" return.** + `SsaFunction` now tracks whether a checked edit mutated it, and + `PassTransaction::run_group` treats a mutated function as changed regardless of + what the pass reported. The precondition that made this necessary — a pass + editing under `SsaRollbackPolicy::Never` must report the change when the edit + fails, or the group skips both verification and rollback — was unstated, + unenforced, and had been got wrong by eight passes. It is now structural + rather than a convention: a pass that mutates and then claims otherwise is + still verified and still rolled back. + +### Added + +- **`SsaFunction::take_edit_dirty`** — reports whether a checked edit mutated the + function, clearing the flag. For consumers driving their own pass groups, who + need the same guarantee `PassTransaction` now provides. + +- **`SsaFunction::refresh_def_sites` is public.** It recomputes every variable's + definition site from the IR as it actually stands. Front ends that build SSA + incrementally cannot always know an instruction's final index while lowering, + and a stale index that runs past the end of its block fails index-bounds + verification. This is the routine `repair_ssa` already used internally. + +### Fixed + +- **Trivial-phi substitution chains in rebuild mode resolved to a deleted + value.** The back-to-front composition introduced in 0.4.1 resolves each source + only through entries already inserted, so for `[(p2, x), (p1, p2)]` it records + `p1 -> p2` while `p2` is retired in the same round. Uses of `p1` were rewritten + to a phi deleted moments later. Each chain is now walked to a target that is + not itself being replaced, with a cycle guard — the repair path had always done + this. The 0.4.1 change remains correct as a performance fix; only its + composition order was wrong. + +- **Self-referential phis were removed without rewriting their uses.** A phi + recorded as `(result, result)` has no other value to substitute and is + deliberately absent from the substitution map, but rebuild mode retired it + anyway, stranding every use on a variable nothing defined. It is now retired + only once nothing reads it, which is the condition the repair path already + applied. + +- **`pre_clean_unreachable` inlined trivial phis without resolving chains.** Its + replacement map was applied entry by entry, so a phi inlined to a value that + was itself another phi being inlined in the same pass left the intermediate in + place — a use naming a phi the same loop had already removed. Because the map + is a `BTreeMap`, whether the chain resolved depended on key order, which made + the failure rare and order-dependent rather than reliably reproducible. + +- **`repair_ssa` did not repair same-block future uses.** An instruction-scope + edit can leave a use naming a definition later in its own block. The rebuild + path repairs exactly this; the repair path did not, so the transactional guard + rejected the result as `IntraBlockCycle` and discarded the pass's work. + +- **Entry replacements were labelled as phi-defined.** The stand-in + `repair_same_block_future_uses` fabricates for a use with no prior definition + is undefined by construction — it represents the value incoming to the + function. Copying the source variable's origin labelled it `Phi`, asserting + that a phi defined it. It now carries `EntryLiveIn`, whose contract is exactly + that the caller supplies it. + +- **`clear_all_phis` discarded phis the rebuild could not reconstruct.** A phi + whose result belongs to no rename group, or to a group with no recorded + definition, cannot be re-placed by `place_phis`; clearing it destroyed its + definition outright. Such phis are now retained. + +- **Phi results and phi-operand uses were invisible to the rebuild's def/use + collection.** `collect_defs` did not record a phi result as a definition of its + group, so a group whose only definition reaching a region was a phi contributed + no block there and no phi was re-placed where one was still needed. + `collect_uses_and_liveness` did not attribute a phi operand to the predecessor + it flows from, understating liveness on that edge. + +- **`expand_phi_predecessor` could leave two operands on one edge.** A + predecessor reaching a block both directly and through the block being bypassed + had an operand added for an edge it already named. Duplicate operands have no + defined meaning and consumers disagree — `PhiNode::operand_from` returns the + first, SCCP meets them all and yields Bottom. The replaced edge is now dropped + and operands are added only for predecessors the phi does not already name. + +- **Seven passes reported "unchanged" after applying edits.** `SsaEditOptions::new()` + defaults to `SsaRollbackPolicy::Never`, so a failed edit or boundary repair + leaves the edits applied. Returning `false`/`0` then tells the pass-group + transaction nothing changed, and its `Unchanged` arm returns *without verifying + and without rolling back* — so damaged IR was kept, and kept unchecked. On a + 125 MB reference binary seven edit sessions failed this way and produced zero + rollbacks, meaning seven functions carried mutated, unverified IR. + `algebraic`, `ranges`, `reassociate`, `strength` and `threading` now report the + change so the transaction verifies and rolls back, as do `controlflow` and + `blockmerge` (below). + + The mixed policy across passes is deliberate and unchanged: passes that verify + inside their own edit session (`copying`, `predicates`, `licm`) need + `OnFailure` for that verification to mean anything, while passes that delegate + to the transaction use `Never` to avoid a second snapshot — the transaction + already clones once per pass, where `OnFailure` clones per edit session. What + was missing was the unstated precondition that a `Never` pass must report the + change on failure. + +- **`gvn` reported "unchanged" only correctly in debug builds.** Its rollback + policy is `OnFailure` under `debug_assertions` and `Never` otherwise, so + `return 0` on failure was true under test and false in release — the one + configuration where the damaged IR would ship. The return now depends on which + policy actually ran. + +- **`controlflow` and `blockmerge` reported "unchanged" after applying edits.** + Under `SsaRollbackPolicy::Never` a failed boundary repair leaves the edits in + place. Reporting zero told the caller nothing had changed, and a pass-group + transaction treats "unchanged" as "nothing to verify" — so damaged IR was kept, + and kept unchecked. Both now report the applied edits, which lets the + transaction verify the function and roll it back. + +### Ownership + +- Recorded ATRAPS LLC as copyright holder and added a `NOTICE` file. The Apache-2.0 + appendix was never filled in — it still carried the literal + `[yyyy] [name of copyright owner]` placeholder, so nothing in this repo stated + who owned it. +- Added a `repository` field. The manifest declared `documentation` but no + repository, so crates.io showed no source link for any published version. +- Dropped the deprecated `authors` field. +- Publishing now uses crates.io trusted publishing instead of a stored registry token. + ## [0.4.1] - 2026-07-26 Speculative evaluation for `SsaEvaluator`. Consumers that explore alternative diff --git a/Cargo.toml b/Cargo.toml index a2b5c67..25387f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,11 +1,11 @@ [package] name = "analyssa" -version = "0.4.1" +version = "0.5.0" edition = "2024" -authors = ["Johann Kempter "] rust-version = "1.88" license = "Apache-2.0" description = "Target-agnostic SSA IR, analyses, and optimization pipeline" +repository = "https://github.com/ATRAPSLLC/analyssa" documentation = "https://docs.rs/analyssa" readme = "README.md" keywords = ["ssa", "ir", "compiler", "analysis", "optimization"] @@ -45,20 +45,20 @@ indexing_slicing = "deny" serde = ["dep:serde"] [dependencies] -thiserror = "2.0.19" +thiserror = "2.0.20" boxcar = "0.2.14" dashmap = "6.2.1" rayon = "1.12.0" log = "0.4.33" num_enum = "0.7.6" -serde = { version = "1.0.228", features = ["derive"], optional = true } +serde = { version = "1.0.229", features = ["derive"], optional = true } [dev-dependencies] # Exercises the `serde` feature's round-trip guarantee (tests/serde.rs). serde_json = "1.0.151" # Measures the SSA maintenance costs the optimization pipeline pays per pass # (benches/ssa_repair.rs). -criterion = "0.8" +criterion = "0.8.2" [[bench]] name = "ssa_repair" diff --git a/LICENSE b/LICENSE index 261eeb9..d4ab6aa 100644 --- a/LICENSE +++ b/LICENSE @@ -186,7 +186,7 @@ same "printed page" as the copyright notice for easier identification within third-party archives. - Copyright [yyyy] [name of copyright owner] + Copyright 2026 ATRAPS LLC Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/NOTICE b/NOTICE new file mode 100644 index 0000000..90dd5aa --- /dev/null +++ b/NOTICE @@ -0,0 +1,24 @@ +analyssa +Copyright 2026 ATRAPS LLC + +This product includes software developed by ATRAPS LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +---- + +This project includes the following third-party software: + +Third-party dependencies are listed in Cargo.toml and their licenses +can be found in their respective repositories. All dependencies are +compatible with the Apache 2.0 license. diff --git a/README.md b/README.md index de8b253..b2f5064 100644 --- a/README.md +++ b/README.md @@ -84,4 +84,5 @@ See [`CHANGELOG.md`](CHANGELOG.md). ## License -Apache-2.0. See [`LICENSE`](LICENSE). +Copyright 2026 ATRAPS LLC. Licensed under the Apache License, +Version 2.0. See [`LICENSE`](LICENSE) and [`NOTICE`](NOTICE). diff --git a/src/ir/function/editor.rs b/src/ir/function/editor.rs index 371b6ef..f5ed046 100644 --- a/src/ir/function/editor.rs +++ b/src/ir/function/editor.rs @@ -283,11 +283,18 @@ impl SsaFunction { } }; - if report.changed - && let Err(error) = finish_edit_scope(self, report.scope) - { - restore_on_failure(self, original); - return Err(error); + if report.changed { + // Set before boundary repair can fail. Under + // `SsaRollbackPolicy::Never` a failed repair leaves these edits + // applied, and the caller's own "changed" return has repeatedly + // been written to say otherwise — so the pass group reads this + // instead of trusting it. A rollback below replaces the whole + // function with its pre-edit clone, which clears the flag. + self.mark_edit_dirty(); + if let Err(error) = finish_edit_scope(self, report.scope) { + restore_on_failure(self, original); + return Err(error); + } } if options.verify { @@ -922,9 +929,9 @@ impl<'a, T: Target> SsaEditor<'a, T> { let Some(block) = self.ssa.block_mut(block_idx) else { return Err(Error::new(format!("missing block B{block_idx}"))); }; - let Some((&first_pred, extra_preds)) = new_preds.split_first() else { + if new_preds.is_empty() { return Ok(0); - }; + } let mut updated = 0usize; for phi in block.phi_nodes_mut() { @@ -937,16 +944,33 @@ impl<'a, T: Target> SsaEditor<'a, T> { continue; }; - if let Some(operand) = phi - .operands_mut() - .iter_mut() - .find(|operand| operand.predecessor() == old_pred) - { - operand.set_predecessor(first_pred); - updated = updated.saturating_add(1); - } + // Drop the edge being replaced, then re-add one operand per new + // predecessor — but never for a predecessor the phi already names. + // + // A predecessor can reach this block *both* directly and through the + // block being bypassed; redirecting then collapses the two paths onto + // one edge. Adding an operand regardless leaves two operands for that + // single edge, which has no defined meaning and which consumers + // disagree about: `PhiNode::operand_from` returns the first and + // discards the rest, while SCCP meets them all and yields Bottom. + // `retarget_phi_predecessor` already guards this case; this is the + // same guard for the one-to-many form. + // + // When an operand for that predecessor already exists it is kept as + // it stands: it describes the direct edge, which is the edge that + // survives. + phi.operands_mut() + .retain(|operand| operand.predecessor() != old_pred); + updated = updated.saturating_add(1); - for &pred in extra_preds { + for &pred in new_preds { + if phi + .operands() + .iter() + .any(|operand| operand.predecessor() == pred) + { + continue; + } phi.add_operand(PhiOperand::new(value, pred)); updated = updated.saturating_add(1); } diff --git a/src/ir/function/mod.rs b/src/ir/function/mod.rs index e37bac5..8584a3b 100644 --- a/src/ir/function/mod.rs +++ b/src/ir/function/mod.rs @@ -243,6 +243,23 @@ pub struct SsaFunction { /// Defaults to [`FunctionKind::Normal`]. Set during SSA construction by /// frontends that need to mark functions as interrupt service routines. kind: FunctionKind, + + /// Whether a checked edit has mutated this function since the flag was last + /// taken. + /// + /// [`Self::edit`] sets this the moment an edit session reports a change, + /// *before* boundary repair can fail. A session running under + /// [`SsaRollbackPolicy::Never`] + /// leaves its edits applied when repair fails, so the caller's own + /// "did anything change" return cannot be trusted to say so — and a pass + /// group that believes nothing changed skips both verification and + /// rollback. Consumers read this instead of trusting that return; see + /// [`Self::take_edit_dirty`]. + /// + /// A rollback replaces the whole function with its pre-edit clone, which + /// restores this to whatever it was before the edit, so a restored failure + /// correctly reports clean. + edit_dirty: bool, } impl Clone for SsaFunction { @@ -261,6 +278,7 @@ impl Clone for SsaFunction { exception_handlers, rename_groups, kind, + edit_dirty, } = self; Self { blocks: blocks.clone(), @@ -276,6 +294,7 @@ impl Clone for SsaFunction { exception_handlers: exception_handlers.clone(), rename_groups: rename_groups.clone(), kind: *kind, + edit_dirty: *edit_dirty, } } @@ -306,6 +325,7 @@ impl Clone for SsaFunction { exception_handlers, rename_groups, kind, + edit_dirty, } = source; self.blocks.clone_from(blocks); self.variables.clone_from(variables); @@ -321,6 +341,7 @@ impl Clone for SsaFunction { self.exception_handlers.clone_from(exception_handlers); self.rename_groups.clone_from(rename_groups); self.kind = *kind; + self.edit_dirty = *edit_dirty; } } @@ -351,6 +372,7 @@ impl SsaFunction { exception_handlers: Vec::new(), rename_groups: Vec::new(), kind: FunctionKind::Normal, + edit_dirty: false, } } @@ -387,6 +409,7 @@ impl SsaFunction { exception_handlers: Vec::new(), rename_groups: Vec::with_capacity(var_capacity), kind: FunctionKind::Normal, + edit_dirty: false, } } @@ -1078,6 +1101,24 @@ impl SsaFunction { self.kind } + /// Returns whether a checked edit has mutated this function, clearing the + /// flag. + /// + /// A pass group must treat a `true` result as "changed" regardless of what + /// the pass itself reported: under + /// [`SsaRollbackPolicy::Never`] + /// a failed boundary repair leaves the edits applied, and a pass that + /// returns "unchanged" would otherwise have its damaged IR kept without + /// verification. + pub fn take_edit_dirty(&mut self) -> bool { + core::mem::replace(&mut self.edit_dirty, false) + } + + /// Marks this function as mutated by a checked edit. + pub(in crate::ir::function) fn mark_edit_dirty(&mut self) { + self.edit_dirty = true; + } + /// Sets the function kind. /// /// # Arguments diff --git a/src/ir/function/rebuild.rs b/src/ir/function/rebuild.rs index 9c7299e..6b87eb2 100644 --- a/src/ir/function/rebuild.rs +++ b/src/ir/function/rebuild.rs @@ -248,6 +248,9 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { /// May panic on internal assertion failures (debug builds) if intermediate /// invariants are violated, indicating a bug in a preceding pass. pub fn rebuild(&mut self) -> Result<()> { + // Before any phase runs: is the SSA the pass handed us already carrying + // a dangling phi operand? The earlier `PREREBUILD` probe scanned + // instruction operands only and could not see one. // Stage 1: Pre-clean self.pre_clean_unreachable(); // Phase 1 self.recompute_groups_from_connectivity(); // Phase 2 @@ -500,7 +503,29 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { // Apply replacements: substitute phi result uses with the single operand if !replacements.is_empty() { - let replacement_map: BTreeMap = replacements.into_iter().collect(); + let direct: BTreeMap = replacements.into_iter().collect(); + // Resolve chains before substituting. A phi inlined to `first` can + // name another phi being inlined in the same pass, and applying the + // map entry-by-entry leaves that intermediate in place — a use then + // names a phi this loop has already removed, which is `UndefinedUse`. + // Iterating a `BTreeMap` makes it order-dependent rather than + // reliably wrong, so it survives wherever key order happens to + // resolve the chain. + let replacement_map: BTreeMap = direct + .iter() + .filter_map(|(from, to)| { + let mut current = *to; + let mut visited: BTreeSet = BTreeSet::new(); + visited.insert(*from); + while let Some(&next) = direct.get(¤t) { + if !visited.insert(current) { + break; + } + current = next; + } + (current != *from).then_some((*from, current)) + }) + .collect(); for block in &mut self.ssa.blocks { for instr in block.instructions_mut() { for (&old_var, &new_var) in &replacement_map { @@ -1316,12 +1341,34 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { self.defs.entry(group).or_default().insert(0); } - // Collect defs from instructions using group IDs. + // Collect defs from instructions and phi results using group IDs. + // + // A phi result is a definition of its group in its block, and it has to + // seed the def set like any other: `place_phis` derives phi placement + // from the iterated dominance frontier *of these blocks*, and + // `clear_all_phis` has already dissolved the phis themselves by then. + // + // Omitting them means a group whose only definition reaching some region + // was a phi contributes no block there, so no phi is re-placed where one + // is still needed — and the uses of the dissolved result are left naming + // a variable nothing defines. That is `UndefinedUse`, and it is what + // `rename` was observed to introduce on 4,237 functions of the reference + // sample despite the IR being valid on entry to the rebuild. + // + // Cytron places a phi wherever a definition's dominance frontier reaches, + // and treats a phi as a definition for exactly this reason: phi placement + // is a fixpoint, so a phi can be what justifies the next one. for block in &self.ssa.blocks { let block_idx = block.id(); if !self.reachable.contains(block_idx) { continue; } + for phi in block.phi_nodes() { + let group = self.ssa.rename_group(phi.result()); + if group != u32::MAX { + self.defs.entry(group).or_default().insert(block_idx); + } + } for instr in block.instructions() { for dest in instr.defs() { let group = self.ssa.rename_group(dest); @@ -1359,6 +1406,33 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { if !self.reachable.contains(block_idx) { continue; } + // A phi operand is a use, recorded against the **predecessor** it + // arrives on: the value must be live at the end of that edge, not at + // the head of this block. + // + // Without this, a group consumed only by phi operands is live-in + // nowhere, so `place_pruned_phis` — which places a phi only where the + // group is live-in — places none for it. `clear_all_phis` has already + // dissolved the original by then, leaving the phi-operand uses naming + // a variable nothing defines: `UndefinedUse`. Recording phi results in + // `collect_defs` alone does not help, because this liveness gate still + // suppresses the placement. + for phi in block.phi_nodes() { + for operand in phi.operands() { + let group = self.ssa.rename_group(operand.value()); + if group == u32::MAX { + continue; + } + let pred = operand.predecessor(); + if !self.reachable.contains_checked(pred) { + continue; + } + use_sites + .entry(group) + .or_insert_with(|| BitSet::new(block_count)) + .insert(pred); + } + } for instr in block.instructions() { instr.op().for_each_use(|use_var| { let group = self.ssa.rename_group(use_var); @@ -1418,10 +1492,45 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { ); } - /// Clears all phi nodes from all blocks before fresh placement. + /// Clears the phi nodes this rebuild will re-place, before fresh placement. + /// + /// A phi whose result carries **no rename group** (`u32::MAX`) is kept, + /// because nothing downstream can restore it: + /// + /// - [`Self::recompute_groups_from_connectivity`] skips such a phi when it + /// unions operands into groups, and its splitting step only ever + /// subdivides *existing* groups — so an ungrouped result is never given + /// one. + /// - [`Self::place_phis`] places phis per rename group, so it will not + /// re-create a phi belonging to none. + /// - [`Self::rename`] skips ungrouped variables outright, so no rename-map + /// entry is produced and [`Self::apply_rename_map`] has nothing to rewrite + /// the surviving uses with. + /// + /// Clearing it therefore deletes a definition whose uses remain, and the + /// function reaches the verifier with `UndefinedUse` on each one. The + /// CFG-modifying edit path runs under `SsaRollbackPolicy::Never`, so the + /// caller keeps that damaged IR rather than restoring it — which is why this + /// one line accounted for the great majority of reverted optimization passes + /// across `controlflow`, `blockmerge` and `loopcanon`. + /// + /// The rebuild reconstructs SSA *for rename groups*. A phi outside every + /// group is not part of that model, so it is not this pass's to destroy. fn clear_all_phis(&mut self) { + let ungrouped: BTreeSet = self + .ssa + .blocks + .iter() + .flat_map(|block| block.phi_nodes().iter().map(|phi| phi.result())) + .filter(|result| { + let group = self.ssa.rename_group(*result); + group == u32::MAX || !self.defs.contains_key(&group) + }) + .collect(); for block in &mut self.ssa.blocks { - block.phi_nodes_mut().clear(); + block + .phi_nodes_mut() + .retain(|phi| ungrouped.contains(&phi.result())); } } @@ -1623,10 +1732,27 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { existing.insert(op.predecessor()); } let group = self.ssa.rename_group(phi.result()); + // The version-0 entry for this phi's group is the + // natural filler. A phi belonging to no group has no + // such entry — [`Self::clear_all_phis`] retains + // exactly those, because nothing here can re-create + // them — so fall back to a value the phi already + // carries on another edge. + // + // Skipping instead leaves the phi short an operand for + // a real predecessor, which is `MissingPhiOperand`; + // a CFG edit that adds a predecessor to such a block + // is precisely what exposes it. Every operand of a phi + // names the same variable, so reusing one is the same + // choice `expand_phi_predecessor` makes when it + // propagates a value onto newly-redirected edges. + let filler = version_stacks + .get(&group) + .and_then(|stack| stack.first().copied()) + .or_else(|| phi.operands().first().map(PhiOperand::value)); for &pred in &preds { if !existing.contains(pred) - && let Some(&v0) = - version_stacks.get(&group).and_then(|stack| stack.first()) + && let Some(v0) = filler { fixes.push((block_idx, phi_idx, pred, v0)); } @@ -1750,7 +1876,7 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { } /// Replaces instruction operands that point at definitions later in the same block. - fn repair_same_block_future_uses(&mut self) { + pub(in crate::ir::function) fn repair_same_block_future_uses(&mut self) { let mut future_uses: Vec<(usize, usize, SsaVarId)> = Vec::new(); let mut repairs: Vec<(usize, usize, SsaVarId, SsaVarId)> = Vec::new(); let mut entry_replacements: BTreeMap = BTreeMap::new(); @@ -1838,11 +1964,13 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { let group = self.ssa.rename_group(var); if group == u32::MAX { let source = self.ssa.variable(var)?; - let origin = source.origin(); let var_type = source.var_type().clone(); - let replacement = self - .ssa - .create_variable(origin, 0, DefSite::entry(), var_type); + let replacement = self.ssa.create_variable( + VariableOrigin::EntryLiveIn, + 0, + DefSite::entry(), + var_type, + ); let new_group = self.next_group; self.next_group = self.next_group.saturating_add(1); self.ssa.set_rename_group(replacement, new_group); @@ -1868,11 +1996,10 @@ impl<'a, T: Target> SsaRebuilder<'a, T> { } let source = self.ssa.variable(var)?; - let origin = source.origin(); let var_type = source.var_type().clone(); - let replacement = self - .ssa - .create_variable(origin, 0, DefSite::entry(), var_type); + let replacement = + self.ssa + .create_variable(VariableOrigin::EntryLiveIn, 0, DefSite::entry(), var_type); self.ssa.set_rename_group(replacement, group); replacements.insert(group, replacement); Some(replacement) diff --git a/src/ir/function/repair.rs b/src/ir/function/repair.rs index e775d80..abef019 100644 --- a/src/ir/function/repair.rs +++ b/src/ir/function/repair.rs @@ -32,7 +32,7 @@ //! | CFG-modifying (add/remove blocks, change branches) | `rebuild_ssa` | use crate::{ - ir::function::{SsaFunction, TrivialPhiOptions}, + ir::function::{SsaFunction, TrivialPhiOptions, rebuild::SsaRebuilder}, target::Target, }; @@ -72,5 +72,11 @@ impl SsaFunction { self.eliminate_trivial_phis(&TrivialPhiOptions { reachable: None }); self.eliminate_dead_phis(); self.compact_variables(); + // An instruction-scope edit can leave a use naming a definition that + // sits later in the same block. The rebuild path repairs exactly this + // (Phases 17b/18b); without it here, `repair_ssa` returns IR the + // transactional guard rejects as `IntraBlockCycle` and the pass is + // rolled back with its work discarded. + SsaRebuilder::new(self).repair_same_block_future_uses(); } } diff --git a/src/ir/function/transforms.rs b/src/ir/function/transforms.rs index f576d77..8129560 100644 --- a/src/ir/function/transforms.rs +++ b/src/ir/function/transforms.rs @@ -721,13 +721,33 @@ impl SsaFunction { // resolved every later entry is already final, and earlier ones // must not be followed. let resolved = { + // Every substitution target must be a variable that survives + // this round. Composing back-to-front only resolves a source + // through entries already inserted, so a chain + // `p1 -> p2 -> x` can leave `p1 -> p2` while `p2` is retired + // in the same round — the rewrite then points uses of `p1` at + // `p2`, which is deleted moments later, and those uses are + // stranded on a variable nothing defines. Walk each chain to + // its end instead, exactly as the repair branch above does. + let direct: BTreeMap = trivial_phis + .iter() + .filter(|(result, source)| result != source) + .copied() + .collect(); let mut resolved: BTreeMap = BTreeMap::new(); - for (result, source) in trivial_phis.iter().rev() { - if result == source { - continue; + for (result, source) in &direct { + let mut current = *source; + let mut visited: BTreeSet = BTreeSet::new(); + visited.insert(*result); + while let Some(&next) = direct.get(¤t) { + if !visited.insert(current) { + break; + } + current = next; + } + if current != *result { + resolved.insert(*result, current); } - let target = resolved.get(source).copied().unwrap_or(*source); - resolved.insert(*result, target); } resolved }; @@ -748,10 +768,24 @@ impl SsaFunction { } } + // A self-referential phi is recorded as `(result, result)` and is + // deliberately absent from `resolved` — there is no other value + // to rewrite its uses to. Removing it regardless strands every + // one of those uses on a variable nothing defines. Retire it + // only once nothing reads it; a later fixpoint round collects it + // after its readers go away. The repair branch already applies + // exactly this condition. + let still_read = self.collect_read_variables(); let mut trivial_set = BitSet::new(variable_count); - for (result, _) in &trivial_phis { + for (result, source) in &trivial_phis { + if result == source && still_read.contains_checked(result.index()) { + continue; + } trivial_set.insert(result.index()); } + if trivial_set.is_empty() { + break; + } total_eliminated = total_eliminated.saturating_add(trivial_set.count()); for block in &mut self.blocks { block.phi_nodes_mut().retain(|phi| { @@ -818,7 +852,18 @@ impl SsaFunction { read } - pub(in crate::ir::function) fn refresh_def_sites(&mut self) { + /// Recomputes every variable's definition site from its current position. + /// + /// A definition site names the block and the index of the phi or + /// instruction that produces the variable. Any edit that inserts, removes, + /// or reorders instructions invalidates those indices, and a stale index + /// that runs past the end of its block is rejected by index-bounds + /// verification. This restores them from the IR as it actually stands. + /// + /// Variables with no remaining definition are reset to an entry site unless + /// something still reads them, so a destroyed definition is not disguised as + /// a legitimate entry value. + pub fn refresh_def_sites(&mut self) { let variable_count = self.var_id_capacity(); let mut active_defs = BitSet::new(variable_count); diff --git a/src/passes/algebraic.rs b/src/passes/algebraic.rs index 375036b..4755789 100644 --- a/src/passes/algebraic.rs +++ b/src/passes/algebraic.rs @@ -176,7 +176,13 @@ where }); if result.is_err() { - return false; + // The session runs under `SsaRollbackPolicy::Never`, so a failed edit or + // boundary repair leaves the edits applied — the function is mutated and + // possibly mid-repair. Reporting "unchanged" would make the pass-group + // transaction skip **both** verification and rollback, keeping damaged + // IR and keeping it unchecked. Report the change so the transaction + // verifies this function and rolls it back. + return true; } changed diff --git a/src/passes/blockmerge.rs b/src/passes/blockmerge.rs index a1f452c..5e180b9 100644 --- a/src/passes/blockmerge.rs +++ b/src/passes/blockmerge.rs @@ -183,7 +183,13 @@ where Ok(()) }); if result.is_err() { - return 0; + // The session runs under [`SsaRollbackPolicy::Never`], so a failed + // boundary repair leaves these edits applied. Reporting zero would say + // nothing changed, and a pass-group transaction skips both verification + // and rollback on that report — so damaged IR is kept, and kept + // unchecked. Report the applied edits (floor of one) so the transaction + // verifies this function and rolls it back. + return redirected.saturating_add(cleared).max(1); } redirected.saturating_add(cleared) } @@ -355,7 +361,11 @@ where Ok(()) }); if result.is_err() { - return false; + // Edits already applied and not rolled back (see the session policy + // above). `false` here means "unchanged" to the caller, which makes + // the pass-group transaction skip verification and keep the damaged + // IR. Report the change so it is verified and rolled back instead. + return true; } let event = crate::events::Event { kind: EventKind::BlockRemoved, diff --git a/src/passes/controlflow.rs b/src/passes/controlflow.rs index 39f17bb..791c08c 100644 --- a/src/passes/controlflow.rs +++ b/src/passes/controlflow.rs @@ -145,7 +145,24 @@ where }); if result.is_err() { - return 0; + // The session runs under [`SsaRollbackPolicy::Never`], so a failure in + // boundary repair or rebuild leaves the edits already applied — the + // function is mutated, and possibly mid-repair. + // + // Reporting zero here would say the opposite. `run` would then report + // `changed == false` to its caller, and a pass-group transaction treats + // "nothing changed" as "nothing to check": its `Unchanged` arm returns + // **without verifying and without rolling back**, on the reasoning that a + // pass which changed nothing cannot have broken anything. That reasoning + // holds only while this return value is truthful, so a zero here keeps + // damaged IR and hides it from the one check that would have caught it. + // + // Report the edits that were applied instead. `total_changes` is exact — + // the closure itself is infallible, so any error comes from the repair + // that runs *after* the counted mutations — and the floor of one covers + // a mutation no counter observed. Non-zero is what makes the transaction + // verify this function and roll it back. + return total_changes.max(1); } total_changes diff --git a/src/passes/gvn.rs b/src/passes/gvn.rs index 03d9597..d1b96c7 100644 --- a/src/passes/gvn.rs +++ b/src/passes/gvn.rs @@ -323,7 +323,17 @@ where ); if edit_result.is_err() { - return 0; + // The rollback policy differs by build: debug restores the function, so + // nothing changed and zero is the truth. Release runs under + // `SsaRollbackPolicy::Never`, where the edits stay applied — reporting + // zero there would make the pass-group transaction skip both + // verification and rollback, keeping damaged IR and keeping it + // unchecked. Report the change so the transaction handles it. + return if matches!(rollback, SsaRollbackPolicy::OnFailure) { + 0 + } else { + total_replaced.max(1) + }; } total_replaced diff --git a/src/passes/ranges.rs b/src/passes/ranges.rs index 92cf853..17a219c 100644 --- a/src/passes/ranges.rs +++ b/src/passes/ranges.rs @@ -412,7 +412,13 @@ where }); if result.is_err() { - return false; + // The session runs under `SsaRollbackPolicy::Never`, so a failed edit or + // boundary repair leaves the edits applied — the function is mutated and + // possibly mid-repair. Reporting "unchanged" would make the pass-group + // transaction skip **both** verification and rollback, keeping damaged + // IR and keeping it unchecked. Report the change so the transaction + // verifies this function and rolls it back. + return true; } changed diff --git a/src/passes/reassociate.rs b/src/passes/reassociate.rs index be7d3f0..5e187a9 100644 --- a/src/passes/reassociate.rs +++ b/src/passes/reassociate.rs @@ -552,7 +552,13 @@ where }); if result.is_err() { - return false; + // The session runs under `SsaRollbackPolicy::Never`, so a failed edit or + // boundary repair leaves the edits applied — the function is mutated and + // possibly mid-repair. Reporting "unchanged" would make the pass-group + // transaction skip **both** verification and rollback, keeping damaged + // IR and keeping it unchecked. Report the change so the transaction + // verifies this function and rolls it back. + return true; } changed diff --git a/src/passes/strength.rs b/src/passes/strength.rs index 67aef43..c32bb1d 100644 --- a/src/passes/strength.rs +++ b/src/passes/strength.rs @@ -414,7 +414,13 @@ where }); if result.is_err() { - return false; + // The session runs under `SsaRollbackPolicy::Never`, so a failed edit or + // boundary repair leaves the edits applied — the function is mutated and + // possibly mid-repair. Reporting "unchanged" would make the pass-group + // transaction skip **both** verification and rollback, keeping damaged + // IR and keeping it unchecked. Report the change so the transaction + // verifies this function and rolls it back. + return true; } changed diff --git a/src/passes/threading.rs b/src/passes/threading.rs index 1298540..95ab39a 100644 --- a/src/passes/threading.rs +++ b/src/passes/threading.rs @@ -127,7 +127,13 @@ where Ok(()) }); if result.is_err() { - return false; + // The session runs under `SsaRollbackPolicy::Never`, so a failed edit or + // boundary repair leaves the edits applied — the function is mutated and + // possibly mid-repair. Reporting "unchanged" would make the pass-group + // transaction skip **both** verification and rollback, keeping damaged + // IR and keeping it unchecked. Report the change so the transaction + // verifies this function and rolls it back. + return true; } changed } diff --git a/src/scheduling/transaction.rs b/src/scheduling/transaction.rs index 22c37f3..3e3a8e8 100644 --- a/src/scheduling/transaction.rs +++ b/src/scheduling/transaction.rs @@ -152,7 +152,16 @@ impl PassTransaction { match outcome { Ok(changed) => { - if !changed { + // `changed` is what the pass *claims*. A pass whose edit session + // ran under `SsaRollbackPolicy::Never` and whose boundary repair + // failed has left its edits applied, and several passes have + // reported "unchanged" in exactly that case — which would take + // the early return below and skip both verification and + // rollback, keeping damaged IR unchecked. `take_edit_dirty` + // reports what actually happened to the function, so the claim + // cannot suppress the check. + let mutated = ssa.take_edit_dirty(); + if !changed && !mutated { return GroupOutcome::Unchanged; } if SsaVerifier::new(ssa) @@ -207,7 +216,7 @@ impl PassTransaction { mod tests { use super::*; use crate::{ - ir::{instruction::SsaInstruction, ops::SsaOp}, + ir::{function::SsaEditOptions, instruction::SsaInstruction, ops::SsaOp}, testing::{self, MockTarget}, }; @@ -252,6 +261,38 @@ mod tests { assert_eq!(ssa.validate(), Ok(())); } + /// A pass that mutates through a checked edit and then reports "unchanged" + /// must not escape verification. + /// + /// `SsaEditOptions::new()` defaults to `SsaRollbackPolicy::Never`, so the + /// edits stay applied when the session fails. Several passes have returned + /// `false` in exactly that case, which — before the function tracked its own + /// mutation — took the `Unchanged` path and kept the damaged IR without + /// verifying it. + #[test] + fn a_pass_that_mutates_then_claims_unchanged_is_still_verified() { + let mut ssa = testing::const_i32_return(7); + let original_blocks = ssa.block_count(); + let mut transaction = PassTransaction::::new(); + + let outcome = transaction.run_group(&mut ssa, |ssa| { + let _ = ssa.edit(SsaEditOptions::new(), |editor| { + editor.insert_before_terminator(0, SsaInstruction::new((), SsaOp::Nop))?; + Ok(()) + }); + // The lie under test. + false + }); + + assert_ne!( + outcome, + GroupOutcome::Unchanged, + "a mutated function must not be reported as unchanged" + ); + assert_eq!(ssa.block_count(), original_blocks); + assert_eq!(ssa.validate(), Ok(())); + } + /// A group that corrupts the IR is rolled back to its pre-group state. #[test] fn an_invalid_group_is_rolled_back() {