Skip to content

[IR Container] Phase 2.5 Copy-Move Semantics#5964

Open
mdavis36 wants to merge 2 commits intomd/phase2-per-fusionfrom
md/phase2-copy-move
Open

[IR Container] Phase 2.5 Copy-Move Semantics#5964
mdavis36 wants to merge 2 commits intomd/phase2-per-fusionfrom
md/phase2-copy-move

Conversation

@mdavis36
Copy link
Collaborator

@mdavis36 mdavis36 commented Feb 12, 2026

Summary

Implement shared-container-aware copy, move, and swap operations, plus per-Fusion name counters that ensure cloned Vals get matching names. This PR combines the originally planned Tasks 3 and 4 — per-Fusion name counters were required to fix CI failures from the copy implementation (553 failures from duplicate TV names when name counter synchronization was missing).

Changes

Copy semantics:

  • Copy constructor: Share container pointer via shared_ptr, register with container, delegate to Fusion::copy
  • Fusion::copy: Clear destination, create IrCloner targeting dest, clone source's deterministic_vals into shared container, clone Fusion-level state (inputs, outputs, axioms, metadata)

Move semantics:

  • Move constructor: Create empty Fusion, swap with source
  • Move assignment: Clear, swap

Swap:

  • Ownership-filtered pointer swap handling three distinct cases:
    1. Two Fusions with different containers
    2. Two Fusions sharing the same container
    3. Swap with third-party Fusions sharing a container

Per-Fusion name counters:

  • val_type_name_map_ and expr_name_counter_ added as Fusion members
  • getValName(ValType) and getExprName() methods on Fusion
  • Counter lifecycle: sync in copy, swap in swap, reset in clear

Copy Semantics in Detail

BEFORE:
  Fusion A ──→ shared_ptr<Container C> ──→ {val_0(A), val_1(A), expr_0(A)}
  Container C: sharing_fusions_ = {A}

COPY: Fusion B(A)   // copy constructor

AFTER:
  Fusion A ─┐
             ├──→ shared_ptr<Container C> ──→ {val_0(A), val_1(A), expr_0(A),
  Fusion B ─┘                                  val_0'(B), val_1'(B), expr_0'(B)}
  Container C: sharing_fusions_ = {A, B}

  // B's clones have matching names: val_0'->name() == val_0->name()
  // IR graphs are independent: modifying B's clone doesn't affect A

The copy constructor shares the container (increments shared_ptr refcount), then clones A's nodes into the same shared storage. Per-Fusion tracking ensures each Fusion's accessors still return only their own nodes.

Swap: Three Cases

Case 1: Different containers
  BEFORE:  A ──→ C1 ──→ {val_0(A)}     B ──→ C2 ──→ {val_1(B)}
  AFTER:   A ──→ C2 ──→ {val_1(A)}     B ──→ C1 ──→ {val_0(B)}
  Statement pointers updated: val_0→B, val_1→A

Case 2: Same container
  BEFORE:  A ─┐                         B ─┐
              ├──→ C ──→ {val_0(A), val_1(B)}
  AFTER:   A ─┐                         B ─┐
              ├──→ C ──→ {val_0(B), val_1(A)}
  Container pointer swap is a no-op; ownership flips.

Case 3: Third-party sharing
  BEFORE:  A ─┐
              ├──→ C1 ──→ {val_0(A), val_2(X)}     B ──→ C2 ──→ {val_1(B)}
         X ─┘
  AFTER:   A ──→ C2 ──→ {val_1(A)}
          B ─┐
              ├──→ C1 ──→ {val_0(B), val_2(X)}
         X ─┘
  Critical: X's statements are NEVER modified.

Why Name Counters Were Merged Into This PR

The initial implementation of Fusion::copy replaced the old IrContainer::copy with direct IrCloner-based cloning but dropped name counter synchronization. Without per-Fusion counters, cloned Vals in a shared container received names starting past the source's last name (e.g., T10–T19 instead of T0–T9), breaking alias_memory.cpp (duplicate tv->name() assertions) and cascading into 553 CI failures across codegen, validation, and numerical checks.

The fix — per-Fusion name counters as Fusion members — is architecturally cleaner than the originally planned IrContainer-level maps, avoids indirection, and aligns with the per-Fusion state model established in earlier tasks.

Relationship to Phase 2

Copy/move/swap are the operations that make shared containers usable. Without them, the shared_ptr and tracking infrastructure from PRs 1–2 are inert. This PR enables the core Phase 2 scenario:

SegmentedFusion::makeFusion (Phase 2 — separate containers):
  auto fusion_segment = make_unique<Fusion>();     // New container
  Fusion::copy(completeFusion(), fusion_segment);  // Clone into separate container

SegmentedFusion::makeFusion (Phase 3 — shared containers):
  auto fusion_segment = make_unique<Fusion>(*completeFusion());  // Copy ctor → shared!
  // Scalars reused, non-scalars cloned into shared container

Phase 2 establishes the copy/move/swap mechanics. Phase 3 simply changes makeFusion from default-ctor + Fusion::copy to copy-ctor (shared container), and the infrastructure from this PR handles everything correctly.

Per-Fusion name counters are critical for cross-clone name correspondence required by GreedyParams::at(tv->name()) and normalization_utils — both of which look up Vals by name as a map key across clone boundaries.

CI Risk

Medium. Copy/move/swap are well-defined operations with clear semantics. The 553-failure CI regression from missing name counters was identified and fixed before merge.

@mdavis36
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented Feb 12, 2026

Review updated until commit 192fd55

Description

  • Copy constructor now shares source container pointer instead of creating new one

  • Fusion::swap rewritten for pointer-based swapping with per-Fusion ownership tracking

  • Fusion::copy clones directly from per-Fusion filtered vals, syncing name counters

  • Name counters moved from IrContainer to Fusion for independent tracking per Fusion

Changes walkthrough

Relevant files
Enhancement
fusion.cpp
Fusion copy/move/swap semantics overhaul                                 

csrc/fusion.cpp

  • Completely rewrote Fusion::swap for pointer-based container swapping
    with ownership tracking
  • Modified Fusion::copy to clone directly from per-Fusion vals and sync
    name counters
  • Updated copy constructor to share source container pointer
  • Added name counter synchronization in copy process to prevent TV name
    collisions
  • Enhanced clear() to reset new Fusion-level name counters
  • +101/-38
    fusion.h
    Fusion name counter members and methods                                   

    csrc/fusion.h

  • Added val_type_name_map_ and expr_name_counter_ members to Fusion
    class
  • Added getValName() and getExprName() methods for per-Fusion name
    tracking
  • Moved name counter logic from IrContainer to Fusion level
  • +18/-0   

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Memory Management

    The copy constructor now shares containers between Fusion objects (line 316: ir_container_(other.ir_container_)). This creates potential lifetime issues - if one Fusion is destroyed while others still reference the shared container, it could lead to dangling pointers or premature container destruction. Need to verify proper reference counting and lifetime management.

    // Copy constructor -- shares the source's container
    Fusion::Fusion(const Fusion& other) : ir_container_(other.ir_container_) {
      FUSER_PERF_SCOPE("Fusion copy");
      ir_container_->addFusion(this);
      Fusion::copy(&other, this);
    }
    Swap Complexity

    The swap operation (lines 107-194) has become significantly more complex with pointer-based swapping and manual ownership tracking. The logic handles same-container vs different-container cases, but the complexity increases risk of bugs. Need thorough testing of edge cases like self-swap, empty containers, and mixed ownership scenarios.

    void Fusion::swap(Fusion& a, Fusion& b) noexcept {
      FUSER_PERF_SCOPE("Fusion swap");
    
      if (&a == &b) {
        return;
      }
    
      // Collect statements owned by each Fusion BEFORE swap
      std::vector<Val*> a_owned_vals, b_owned_vals;
      std::vector<Expr*> a_owned_exprs, b_owned_exprs;
    
      if (a.ir_container_) {
        const auto& av = a.ir_container_->valsOwnedBy(&a);
        const auto& ae = a.ir_container_->exprsOwnedBy(&a);
        a_owned_vals.assign(av.begin(), av.end());
        a_owned_exprs.assign(ae.begin(), ae.end());
      }
      if (b.ir_container_) {
        const auto& bv = b.ir_container_->valsOwnedBy(&b);
        const auto& be = b.ir_container_->exprsOwnedBy(&b);
        b_owned_vals.assign(bv.begin(), bv.end());
        b_owned_exprs.assign(be.begin(), be.end());
      }
    
      // Transfer Fusion registrations between containers before pointer swap.
      // After swap, a will own b's container and b will own a's container.
      if (a.ir_container_ && b.ir_container_ &&
          a.ir_container_.get() != b.ir_container_.get()) {
        a.ir_container_->transferFusion(&a, &b);
        b.ir_container_->transferFusion(&b, &a);
      }
    
      // Swap container pointers
      std::swap(a.ir_container_, b.ir_container_);
    
      // Swap all Fusion-level members
      std::swap(a.inputs_, b.inputs_);
      std::swap(a.outputs_, b.outputs_);
      std::swap(a.io_alias_, b.io_alias_);
      std::swap(a.all_tv_uses_valid_, b.all_tv_uses_valid_);
      std::swap(a.is_during_update_uses_, b.is_during_update_uses_);
      std::swap(a.managed_data_, b.managed_data_);
      std::swap(a.managed_named_data_, b.managed_named_data_);
      std::swap(a.expected_dynamic_smem_bytes_, b.expected_dynamic_smem_bytes_);
      std::swap(a.all_tvs_ptr_, b.all_tvs_ptr_);
      std::swap(a.zero_val_, b.zero_val_);
      std::swap(a.one_val_, b.one_val_);
      std::swap(a.true_val_, b.true_val_);
      std::swap(a.false_val_, b.false_val_);
      std::swap(a.magic_zero_val_, b.magic_zero_val_);
      std::swap(a.axioms_, b.axioms_);
      std::swap(a.metadata_, b.metadata_);
      std::swap(a.val_type_name_map_, b.val_type_name_map_);
      std::swap(a.expr_name_counter_, b.expr_name_counter_);
    
      // Update Statement::ir_container_ pointers: a's old statements now belong
      // to b, and b's old statements now belong to a
      for (auto* val : a_owned_vals) {
        val->ir_container_ = &b;
      }
      for (auto* expr : a_owned_exprs) {
        expr->ir_container_ = &b;
      }
      for (auto* val : b_owned_vals) {
        val->ir_container_ = &a;
      }
      for (auto* expr : b_owned_exprs) {
        expr->ir_container_ = &a;
      }
    
      // Update per-Fusion tracking keys in containers
      if (a.ir_container_ && b.ir_container_) {
        if (a.ir_container_.get() == b.ir_container_.get()) {
          // Same container: directly swap per-Fusion tracking entries
          auto* c = a.ir_container_.get();
          std::swap(c->per_fusion_vals_[&a], c->per_fusion_vals_[&b]);
          std::swap(c->per_fusion_exprs_[&a], c->per_fusion_exprs_[&b]);
        } else {
          // Different containers: rename tracking keys to match new owners
          a.ir_container_->transferStatementOwnership(&b, &a);
          b.ir_container_->transferStatementOwnership(&a, &b);
        }
      } else if (a.ir_container_) {
        a.ir_container_->transferStatementOwnership(&b, &a);
      } else if (b.ir_container_) {
        b.ir_container_->transferStatementOwnership(&a, &b);
      }
    }
    Name Collision Risk

    The new per-Fusion name tracking (lines 651-667) with val_type_name_map_ and expr_name_counter_ could lead to name collisions when multiple Fusions share the same container but have different naming states. The copy constructor copies these counters (line 219-220), but the interaction between shared containers and independent naming needs validation.

    // Per-Fusion name counters. Each Fusion independently tracks name assignment
    // so that cloned Fusions get matching names (T0→T0) regardless of whether
    // they share an IrContainer. This is required by downstream consumers that
    // use tv->name() as a map key (alias_memory, GreedyParams, etc.).
    std::unordered_map<ValType, StmtNameType> val_type_name_map_;
    StmtNameType expr_name_counter_ = 0;
    
    StmtNameType getValName(ValType vtype) {
      if (val_type_name_map_.find(vtype) == val_type_name_map_.end()) {
        val_type_name_map_[vtype] = 0;
      }
      return val_type_name_map_[vtype]++;
    }
    
    StmtNameType getExprName() {
      return expr_name_counter_++;
    }

    @mdavis36
    Copy link
    Collaborator Author

    !test

    @mdavis36 mdavis36 changed the title [IR Container] Phase 2 Copy-Move Semantics [IR Container] Phase 2.5 Copy-Move Semantics Feb 18, 2026
    @mdavis36 mdavis36 force-pushed the md/phase2-copy-move branch from 192fd55 to 35b7405 Compare February 18, 2026 03:13
    @mdavis36 mdavis36 force-pushed the md/phase2-per-fusion branch from 33629cb to 8b162d9 Compare February 18, 2026 03:13
    @mdavis36
    Copy link
    Collaborator Author

    !test

    @mdavis36 mdavis36 marked this pull request as ready for review February 18, 2026 06:37
    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Feb 18, 2026

    Greptile Summary

    This PR implements shared-container-aware copy, move, and swap operations, plus per-Fusion name counters to ensure cloned Vals get matching names across clone boundaries.

    Key Changes:

    • Copy constructor now shares the container pointer (via shared_ptr) and registers with the container before delegating to Fusion::copy
    • Fusion::copy reimplemented to use IrCloner directly, cloning source vals into the shared container while maintaining per-Fusion ownership tracking
    • Per-Fusion name counters (val_type_name_map_, expr_name_counter_) moved from IrContainer to Fusion to enable name synchronization across clones
    • Fusion::swap completely rewritten to handle three distinct cases: different containers, shared containers, and one-null-container scenarios with proper ownership transfer
    • Per-Fusion tracking infrastructure added to IrContainer (per_fusion_vals_, per_fusion_exprs_) with ownership-filtered accessor methods

    Issues Already Reported:

    • Missing sharing_fusions_ update when swapping with null container (edge case)
    • noexcept swap performs allocating operations that could throw
    • Redundant find() check in getValName() method

    The implementation correctly handles the complex ownership semantics required for shared containers, with proper statement ownership transfers, pointer updates, and per-Fusion tracking maintenance across all operations.

    Confidence Score: 4/5

    • This PR is safe to merge with close attention to the swap edge cases
    • The core implementation is sound with correct ownership tracking, statement management, and name counter synchronization. The three previously reported issues are edge cases (null container swap) or pre-existing patterns (noexcept allocations, redundant find check) that don't affect typical usage. The 553 CI failures mentioned in the PR description were already identified and fixed by the per-Fusion name counter implementation.
    • Pay close attention to csrc/fusion.cpp - the swap and copy implementations are complex with multiple edge cases

    Important Files Changed

    Filename Overview
    csrc/fusion.h Added per-Fusion name counters (val_type_name_map_, expr_name_counter_) and updated collection accessors to call ownership-filtered methods. Minor style issue in getValName() with redundant find check.
    csrc/fusion.cpp Complete rewrite of swap() (handles 3 cases) and copy() (shares container in copy ctor). Known issues: noexcept swap can throw, missing sharing_fusions_ update for null container case. Registration and removal functions correctly maintain per-Fusion tracking.
    csrc/ir/container.h Added per-Fusion tracking infrastructure (per_fusion_vals_, per_fusion_exprs_) and ownership-filtered accessor methods. Clean interface additions.
    csrc/ir/container.cpp Implemented ownership-filtered accessors, statement ownership transfer, and removal methods. Implementation correctly handles iteration during erasure and empty set cases.

    Flowchart

    %%{init: {'theme': 'neutral'}}%%
    flowchart TD
        Start([Fusion::swap a, b]) --> SelfCheck{a == b?}
        SelfCheck -->|Yes| Return([Return])
        SelfCheck -->|No| Snapshot[Snapshot owned statements<br/>a_owned_vals, b_owned_vals<br/>a_owned_exprs, b_owned_exprs]
        
        Snapshot --> BothNonNull{Both containers<br/>non-null AND<br/>different?}
        BothNonNull -->|Yes| Transfer[transferFusion for both<br/>C1: a→b, C2: b→a]
        BothNonNull -->|No| SwapPtr[Swap container pointers]
        Transfer --> SwapPtr
        
        SwapPtr --> SwapMembers[Swap all Fusion members<br/>inputs, outputs, name counters, etc.]
        SwapMembers --> UpdatePtrs[Update Statement::ir_container_<br/>pointers in snapshots]
        
        UpdatePtrs --> TrackingCheck{Both containers<br/>non-null?}
        TrackingCheck -->|Yes| SameContainer{Same container?}
        TrackingCheck -->|No| OneNull[Handle null container case<br/>transferStatementOwnership]
        
        SameContainer -->|Yes| DirectSwap[Direct swap of<br/>per_fusion tracking maps]
        SameContainer -->|No| TransferOwnership[transferStatementOwnership<br/>for both containers]
        
        DirectSwap --> Return
        TransferOwnership --> Return
        OneNull --> Return
    
    Loading

    Last reviewed commit: 88b2e60

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    4 files reviewed, 3 comments

    Edit Code Review Agent Settings | Greptile

    Comment on lines +131 to 137
    // Transfer Fusion registrations between containers before pointer swap.
    // After swap, a will own b's container and b will own a's container.
    if (a.ir_container_ && b.ir_container_ &&
    a.ir_container_.get() != b.ir_container_.get()) {
    a.ir_container_->transferFusion(&a, &b);
    b.ir_container_->transferFusion(&b, &a);
    }
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Missing sharing_fusions_ update when one container is null

    When only one Fusion has a container (e.g., a has a container and b doesn't), transferFusion is skipped because the guard on line 133 requires both containers to be non-null. After the container pointer swap on line 140, the container's sharing_fusions_ set will still reference the old Fusion pointer instead of the new owner.

    For example, if a has container C and b has null:

    1. transferFusion is skipped (line 133 fails)
    2. After swap: a.ir_container_ = null, b.ir_container_ = C
    3. C's sharing_fusions_ still contains &a instead of &b
    4. inContainer() checks would fail for b's statements

    While this is likely unreachable in practice (all constructors initialize ir_container_), the defensive code path on lines 189-192 for transferStatementOwnership suggests this case was considered. If so, it should also handle transferFusion:

      if (a.ir_container_ && b.ir_container_ &&
          a.ir_container_.get() != b.ir_container_.get()) {
        a.ir_container_->transferFusion(&a, &b);
        b.ir_container_->transferFusion(&b, &a);
      } else if (a.ir_container_ && !b.ir_container_) {
        a.ir_container_->transferFusion(&a, &b);
      } else if (b.ir_container_ && !a.ir_container_) {
        b.ir_container_->transferFusion(&b, &a);
      }
    

    @@ -107,46 +107,90 @@ bool Fusion::sameDefinition(const Fusion& other) const {
    void Fusion::swap(Fusion& a, Fusion& b) noexcept {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    noexcept swap performs allocating operations

    swap is marked noexcept but performs operations that can throw std::bad_alloc:

    • std::vector construction and .assign() on lines 115-128
    • transferFusion does unordered_set::insert (line 135-136)
    • per_fusion_vals_[&a] with operator[] can allocate (line 182)
    • transferStatementOwnership does unordered_set::insert and unordered_map::operator[] (lines 186-192)

    If any of these throw, std::terminate will be called. This is a pre-existing pattern from the old swap, but the new implementation adds more allocating operations (the std::vector snapshots). Consider either removing noexcept or wrapping in try/catch with a fallback.

    Comment on lines +658 to +663
    StmtNameType getValName(ValType vtype) {
    if (val_type_name_map_.find(vtype) == val_type_name_map_.end()) {
    val_type_name_map_[vtype] = 0;
    }
    return val_type_name_map_[vtype]++;
    }
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Consider using try_emplace or direct operator[]

    Minor style nit: the find + operator[] pattern can be simplified. Since operator[] on unordered_map default-constructs the value (0 for StmtNameType) if the key is missing, the find check is redundant:

    Suggested change
    StmtNameType getValName(ValType vtype) {
    if (val_type_name_map_.find(vtype) == val_type_name_map_.end()) {
    val_type_name_map_[vtype] = 0;
    }
    return val_type_name_map_[vtype]++;
    }
    StmtNameType getValName(ValType vtype) {
    return val_type_name_map_[vtype]++;
    }

    This produces identical behavior because StmtNameType (which is unsigned int) is value-initialized to 0 when default-constructed by operator[].

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    @mdavis36
    Copy link
    Collaborator Author

    !test

    Copy constructor now shares the source's container pointer instead of
    creating a new one. Fusion::copy clones directly from per-Fusion
    filtered vals rather than delegating to IrContainer::copy. Swap changed
    from content-based (IrContainer::swap) to pointer-based with per-Fusion
    ownership tracking for both same-container and different-container cases.
    Move val/expr name counters from IrContainer to Fusion so each Fusion
    independently tracks name assignment. This fixes CI failures where
    Fusion::copy left the dest counter at N (number of cloned vals) instead
    of max(name)+1 when source names were non-sequential, causing newly
    created TVs to collide with existing names.
    
    The fix adds val_type_name_map_ and expr_name_counter_ to Fusion, and
    updates registerVal/registerExpr to use the Fusion-level counters.
    Fusion::copy syncs counters from source to dest after cloning.
    Fusion::swap exchanges counters. Fusion::clear resets them.
    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.

    1 participant