Conversation
|
Review updated until commit 56cf217 Description
|
| Relevant files |
|---|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 PR contains tests |
| ⚡ Recommended focus areas for review |
Potential Deadlock Risk
|
|
!test |
2 similar comments
|
!test |
|
!test |
962852b to
0c3409d
Compare
Moved special values (`zero_val_`, `one_val_`, `true_val_`,
`false_val_`, `magic_zero_val_`) from `IrContainer` to the `Fusion`
class. This ensures that with shared containers, each Fusion has its own
special values, preventing ownership conflicts when one Fusion is
destroyed.
**Option Implemented:** Option A (Move Special Values to Fusion) as
recommended in the prompt.
Added private members and public accessors to Fusion class:
```cpp
// Phase 2: Per-Fusion special values
// With shared containers, each Fusion needs its own special values.
// These are raw pointers - memory is owned by IrContainer's vals_up_.
// Destroying this Fusion removes these vals via
removeStatementsOwnedBy().
Val* zero_val_ = nullptr;
Val* one_val_ = nullptr;
Val* true_val_ = nullptr;
Val* false_val_ = nullptr;
NamedScalar* magic_zero_val_ = nullptr;
```
Public accessors:
- `Val* zeroVal()` - Returns Index 0
- `Val* oneVal()` - Returns Index 1
- `Val* falseVal()` - Returns Bool false
- `Val* trueVal()` - Returns Bool true
- `NamedScalar* magicZeroVal()` - Returns magic zero named scalar
- `Val* zeroVal(DataType dtype)` - Returns 0 for specified dtype
- `Val* oneVal(DataType dtype)` - Returns 1 for specified dtype
Implemented lazy creation pattern for all special value accessors:
```cpp
Val* Fusion::zeroVal() {
if (!zero_val_) {
zero_val_ = IrBuilder::createInContainer<Val>(this, 0L,
DataType::Index);
}
return zero_val_;
}
// Similar implementations for oneVal(), falseVal(), trueVal(),
magicZeroVal()
```
Updated `Fusion::clear()` to reset special value pointers:
```cpp
// Reset per-Fusion special values (they'll be recreated lazily if
needed)
// The actual Val objects were removed by removeStatementsOwnedBy above.
zero_val_ = nullptr;
one_val_ = nullptr;
true_val_ = nullptr;
false_val_ = nullptr;
magic_zero_val_ = nullptr;
```
Removed special value members and added documentation comment:
```cpp
// Note: Special values (zero_val_, one_val_, true_val_, false_val_,
// magic_zero_val_) are now per-Fusion, stored in Fusion class.
// This avoids ownership conflicts when multiple Fusions share an
IrContainer.
// See Fusion::zeroVal(), etc. for the per-Fusion implementation.
```
Removed special value accessor implementations (they're now in Fusion).
All call sites were already updated to use `fusion->zeroVal()` instead
of `ir_container()->zeroVal()`. Verified with grep that no call sites
remain using the old pattern.
Added 8 new unit tests for Task 7:
1. **PerFusionSpecialValuesBasic** - Tests that special values are
created and owned by the Fusion
2. **SpecialValuesOwnedByFusion** - Tests that special values are
tracked in `ownedVals()`
3. **SeparateFusionsHaveOwnSpecialValues** - Tests that two Fusions have
different special value objects
4. **DestroyFusionDoesNotAffectOther** - Tests that destroying one
Fusion doesn't affect another's special values
5. **SpecialValuesLazyCreation** - Tests that same value is returned on
repeated calls
6. **AllSpecialValuesPerFusion** - Tests all five special value
accessors
7. **SpecialValuesClearedOnFusionClear** - Tests that `clear()` resets
special values
8. **SpecialValuesWithDtype** - Tests `zeroVal(dtype)` and
`oneVal(dtype)` accessors
```
[==========] Running 34 tests from 3 test suites.
[ PASSED ] 34 tests.
```
```
[==========] Running 26 tests from 1 test suite.
[ PASSED ] 26 tests.
```
Including 8 new Task 7 tests:
- `Phase2ContainerTest.PerFusionSpecialValuesBasic` - PASSED
- `Phase2ContainerTest.SpecialValuesOwnedByFusion` - PASSED
- `Phase2ContainerTest.SeparateFusionsHaveOwnSpecialValues` - PASSED
- `Phase2ContainerTest.DestroyFusionDoesNotAffectOther` - PASSED
- `Phase2ContainerTest.SpecialValuesLazyCreation` - PASSED
- `Phase2ContainerTest.AllSpecialValuesPerFusion` - PASSED
- `Phase2ContainerTest.SpecialValuesClearedOnFusionClear` - PASSED
- `Phase2ContainerTest.SpecialValuesWithDtype` - PASSED
- `csrc/fusion.h` - Added special value members and accessors
- `csrc/fusion.cpp` - Added accessor implementations, updated `clear()`
- `csrc/ir/container.h` - Removed special values, added comment
- `csrc/ir/container.cpp` - Removed accessor implementations
- `tests/cpp/test_phase2_container_sharing.cpp` - Added 8 unit tests
- [x] Each Fusion has its own special values
- [x] Destroying Fusion A doesn't affect Fusion B's special values
- [x] Special value accessors (`zeroVal()`, `oneVal()`, etc.) return
this Fusion's values
- [x] Lazy creation still works (create on first access)
- [x] Smoke tests pass (34/34)
- [x] Unit tests added (8 tests)
- [x] Unit tests pass (26/26 Phase 2 tests)
- [x] Code compiles without errors
- [x] REPORT.md delivered
1. **Memory ownership:** Special values are raw pointers stored in
Fusion, but the actual memory is owned by IrContainer's `vals_up_`. When
a Fusion is destroyed, `removeStatementsOwnedBy()` cleans up these vals.
2. **Lazy creation pattern:** Special values are created on first
access. This matches the original IrContainer behavior and avoids
creating values that aren't needed.
3. **Clear handling:** `Fusion::clear()` now resets special value
pointers to nullptr after `removeStatementsOwnedBy()` removes the actual
Val objects. This ensures lazy recreation works correctly after clear.
4. **Copy/move handling:** Will be addressed in Tasks 5 and 6. This task
just moves the members and accessors.
Moved `axioms_` and `metadata_` from `IrContainer` to the `Fusion` class. This completes the deprecation of `parent_` usage for val-creating methods, which was necessary because `parent_` implies a 1-1 relationship (container → Fusion), but Phase 2 has 1-many (shared containers). Methods that used `parent_` to create vals were moved to Fusion: - `metadataOf(Val*)` - Now uses `v->container()` to get owning Fusion - `axioms()` - Now creates axiom vals owned by `this` Fusion - `assumePositive/assumeNonNegative` - Now adds to `this` Fusion's axioms - Added `axioms_` and `metadata_` private members - Changed method declarations from forwarding to actual implementations - Added includes for `ir/builder.h` and `ir/internal_nodes.h` - Implemented `metadataOf()`, `axioms()`, `assumePositive()`, `assumeNonNegative()` methods - Updated `clear()` to reset `axioms_` and `metadata_` - Removed `metadataOf()`, `axioms()`, `assumePositive()`, `assumeNonNegative()` declarations - Removed `lazyInitAxioms()` declaration - Removed `axioms_` and `metadata_` members - Removed implementations of above methods - Updated `IrContainer::swap` to remove axioms_/metadata_ swapping - Updated `IrContainer::copy` to remove axioms_/metadata_ handling - Updated `IrContainer::clear` to remove axioms_/metadata_ clearing Each Fusion now has its own axioms and metadata cache. This ensures: 1. No ownership conflicts when multiple Fusions share an IrContainer 2. Correct behavior when one Fusion is destroyed (doesn't affect others) 3. Lazy creation pattern preserved (create on first access) This is a prerequisite for the copy/move semantics changes which will swap/transfer these per-Fusion members.
Transitioned Fusion's container ownership from `unique_ptr` to `shared_ptr` with automatic Fusion registration/unregistration during construction/destruction. - Added `#include <memory>` header - Changed `std::unique_ptr<IrContainer> ir_container_` to `std::shared_ptr<IrContainer> ir_container_` - Added `ir_container_ptr()` method to return the shared_ptr directly (for tests that need to hold reference beyond Fusion lifetime) - Updated default constructor to use `std::make_shared<IrContainer>()` and call `addFusion(this)` for registration - Updated destructor to call `removeFusion(this)` before clearing - Added 4 new tests for Task 3: - `BasicFusionLifecycle`: Create Fusion, add inputs/outputs, destroy - verifies no crashes - `FusionAutoRegistration`: Verifies new Fusion automatically registers (sharingCount == 1) - `FusionDestructorCleanup`: Verifies destructor unregisters and cleans up Statements - `ContainerAccessor`: Verifies `ir_container_ptr()` returns valid shared_ptr - Updated Task 2 tests to account for auto-registration in constructor ``` [ PASSED ] Phase2ContainerTest.LockingBasic [ PASSED ] Phase2ContainerTest.ConcurrentReads [ PASSED ] Phase2ContainerTest.FusionRegistration [ PASSED ] Phase2ContainerTest.FusionTransfer [ PASSED ] Phase2ContainerTest.MultipleRegistration [ PASSED ] Phase2ContainerTest.StatementCleanup [ PASSED ] Phase2ContainerTest.BasicFusionLifecycle [ PASSED ] Phase2ContainerTest.FusionAutoRegistration [ PASSED ] Phase2ContainerTest.FusionDestructorCleanup [ PASSED ] Phase2ContainerTest.ContainerAccessor ``` ``` [ PASSED ] 34 tests including: - FusionCopy_CUDA - FusionMove_CUDA - FusionClear_CUDA - All AbstractTensorTest.* - All NVFuserTest.FusionHash* ``` 1. **Auto-registration in constructor**: The Fusion constructor now calls `ir_container_->addFusion(this)` after creating the container. This ensures every Fusion is always tracked. 2. **Auto-unregistration in destructor**: The destructor calls `ir_container_->removeFusion(this)` which: - Decrements the sharing count - Cleans up Statements owned by this Fusion - Works correctly even when container is shared (other Fusions' Statements preserved) 3. **Added `ir_container_ptr()` method**: Returns `std::shared_ptr<IrContainer>` for cases where code needs to hold a reference to the container beyond the Fusion's lifetime (e.g., testing Statement cleanup after Fusion destruction). 4. **Task 2 test updates**: The previous Task 2 tests assumed Fusions weren't auto-registered. Updated them to use separate IrContainer instances for testing the registration mechanism in isolation. | File | Changes | |------|---------| | `csrc/fusion.h` | Added `<memory>` header, changed `unique_ptr` to `shared_ptr`, added `ir_container_ptr()` | | `csrc/fusion.cpp` | Updated constructor (make_shared + addFusion), updated destructor (removeFusion) | | `tests/cpp/test_phase2_container_sharing.cpp` | Added 4 new tests, updated 3 Task 2 tests |
Added Fusion registration and Statement cleanup capabilities to IrContainer. This enables tracking which Fusions share a container and cleaning up Statements when a Fusion is destroyed. Added public methods for Fusion tracking: - `addFusion(Fusion*)` - register a Fusion as sharing this container - `removeFusion(Fusion*)` - unregister and cleanup owned Statements - `transferFusion(Fusion* from, Fusion* to)` - for move operations - `sharingCount()` - number of Fusions sharing this container - `hasMultipleFusions()` - whether multiple Fusions share this container - `sharingFusions()` - get the set of sharing Fusions Added protected members: - `std::unordered_set<Fusion*> sharing_fusions_` - tracks registered Fusions - `removeStatementsOwnedByUnlocked(Fusion*)` - internal cleanup helper Implemented all Fusion tracking methods: - `addFusion()` - inserts Fusion into `sharing_fusions_` (unique_lock) - `removeFusion()` - removes from set and cleans up owned Statements (unique_lock) - `transferFusion()` - atomic transfer of registration (unique_lock) - `sharingCount()`, `hasMultipleFusions()`, `sharingFusions()` - read accessors (shared_lock) - `removeStatementsOwnedByUnlocked()` - iterates vals/exprs and removes those owned by the given Fusion Also updated `swap()` to include `sharing_fusions_` in the swap. The `removeStatementsOwnedByUnlocked()` function removes: 1. All Vals in `vals_up_` owned by the Fusion 2. All shortcut Vals (`zero_val_`, `one_val_`, `true_val_`, `false_val_`, `magic_zero_val_`) if owned by the Fusion 3. All Exprs in `exprs_up_` owned by the Fusion Ownership is determined by checking `statement->container() == fusion`. Made `ir_container()` accessor public (was protected). This is needed for Phase 2 shared_ptr support where external code needs to access the underlying container. Added 4 new tests: - `FusionRegistration` - verifies add/remove counting - `FusionTransfer` - verifies transfer updates tracking correctly - `MultipleRegistration` - verifies multiple Fusions can register - `StatementCleanup` - verifies Statement cleanup on removeFusion --- ``` [ PASSED ] 34 tests. ``` ``` [ PASSED ] 6 tests. - Phase2ContainerTest.LockingBasic - Phase2ContainerTest.ConcurrentReads - Phase2ContainerTest.FusionRegistration - Phase2ContainerTest.FusionTransfer - Phase2ContainerTest.MultipleRegistration - Phase2ContainerTest.StatementCleanup ``` --- | File | Change Type | |------|-------------| | `csrc/ir/container.h` | Added tracking methods and members | | `csrc/ir/container.cpp` | Implemented tracking methods | | `csrc/fusion.h` | Made `ir_container()` public | | `tests/cpp/test_phase2_container_sharing.cpp` | Added 4 tests | --- All tracking methods use the existing `mutex_`: - `addFusion()`, `removeFusion()`, `transferFusion()` - unique_lock (write) - `sharingCount()`, `hasMultipleFusions()`, `sharingFusions()` - shared_lock (read) Statements store their owning Fusion via `ir_container_` member (in `base_nodes.h`). When a Fusion is removed from a shared container: 1. The Fusion is unregistered from `sharing_fusions_` 2. All Statements where `container() == fusion` are removed from the container This ensures that when a Fusion is destroyed, its IR nodes don't pollute the shared container.
Implemented infrastructure for per-Fusion statement tracking so each
Fusion can efficiently access only its own statements when sharing an
IrContainer with other Fusions. This is a prerequisite for copy/move
semantics in later tasks.
1. **Per-Fusion Tracking Data Structures** (`container.h`)
- Added `per_fusion_vals_`: Maps each Fusion to its owned Vals
- Added `per_fusion_exprs_`: Maps each Fusion to its owned Exprs
2. **New IrContainer Methods** (`container.h/cpp`)
- `valsOwnedBy(Fusion*)`: Returns Vals owned by specific Fusion
- `exprsOwnedBy(Fusion*)`: Returns Exprs owned by specific Fusion
- `transferStatementOwnership(Fusion*, Fusion*)`: For move operations
- `removeStatementsOwnedBy(Fusion*)`: Public API to remove Fusion's
statements
3. **New Fusion Accessor Methods** (`fusion.h`)
- `ownedVals()`: Returns only THIS Fusion's Vals (not all in
container)
- `ownedExprs()`: Returns only THIS Fusion's Exprs (not all in
container)
4. **Updated Registration** (`container.cpp`)
- `registerVal()`: Now updates per-Fusion tracking
- `registerExpr()`: Now updates per-Fusion tracking
- `removeVal()`: Now cleans up per-Fusion tracking
- `removeExpr()`: Now cleans up per-Fusion tracking
5. **Updated Fusion::clear()** (`fusion.cpp`)
- Changed from `ir_container()->clear()` (clears entire container)
- To `ir_container_->removeStatementsOwnedBy(this)` (only clears THIS
Fusion's statements)
- Critical for Invariant 4: `Fusion::clear()` must only affect this
Fusion's state
```
[==========] Running 34 tests from 3 test suites.
[ PASSED ] 34 tests.
```
- `AbstractTensorTest.*` (28 tests): PASS
- `Gpu1Test.FusionClear_CUDA`: PASS
- `Gpu1Test.FusionCopy_CUDA`: PASS
- `Gpu1Test.FusionMove_CUDA`: PASS
- `NVFuserTest.FusionHash*` (3 tests): PASS
```
[==========] Running 18 tests from 1 test suite.
[ PASSED ] 18 tests.
```
New tests added for Task 4:
- `PerFusionValsTracking`: Verifies ownedVals() returns only this
Fusion's vals
- `PerFusionExprsTracking`: Verifies ownedExprs() returns only this
Fusion's exprs
- `ValsOwnedByAPI`: Tests IrContainer::valsOwnedBy() API directly
- `ExprsOwnedByAPI`: Tests IrContainer::exprsOwnedBy() API directly
- `RegisterUpdatesPerFusionTracking`: Verifies registration updates
tracking
- `TransferStatementOwnership`: Tests transferStatementOwnership for
moves
- `ClearOnlyAffectsOwnedStatements`: Verifies clear() only affects this
Fusion
- `RemoveStatementsOwnedByAPI`: Tests public removeStatementsOwnedBy API
1. **Thread Safety**: All new methods use existing mutex_ infrastructure
- `valsOwnedBy()`/`exprsOwnedBy()` acquire shared_lock
- `transferStatementOwnership()`/`removeStatementsOwnedBy()` acquire
unique_lock
2. **Empty Set Handling**: Return static empty set when Fusion has no
statements
```cpp
const std::unordered_set<Val*>& IrContainer::valsOwnedBy(Fusion*
fusion) const {
std::shared_lock lock(mutex_);
static const std::unordered_set<Val*> empty;
auto it = per_fusion_vals_.find(fusion);
return it != per_fusion_vals_.end() ? it->second : empty;
}
```
3. **Backward Compatibility**:
- Existing `vals()` and `unordered_exprs()` unchanged
- With single Fusion (Phase 1 pattern): `ownedVals() == vals()`
- With shared container (Phase 2): `ownedVals() ⊆ vals()`
| Level | Vals | Exprs | Description |
|-------|------|-------|-------------|
| **IR Traversal** | `exprs()` | N/A | Reachable from I/O (existing,
unchanged) |
| **All in Container** | `vals()` | `unordered_exprs()` | All in shared
container |
| **Owned by Fusion** | `ownedVals()` | `ownedExprs()` | Only THIS
Fusion's statements (NEW) |
Implemented Phase 2 copy/move semantics where copy shares the container and move uses pointer-based swap. This enables efficient Fusion operations with shared IrContainers. 1. **Pointer-Based Swap** - Swap container shared_ptrs, not contents 2. **Copy Shares Container** - Copy constructor shares IrContainer with source 3. **Move Uses Swap** - Simple `Fusion() + swap` pattern from Phase 1 **Pointer-based swap:** - Collects owned statements before swap - Transfers Fusion registrations between containers - Swaps container pointers (not content!) - Swaps all Fusion-level members including axioms_/metadata_ - Updates statement ownership for only the swapped Fusions - Updates per-Fusion tracking in containers **New copy implementation:** - Creates IrCloner targeting destination Fusion directly - Clones only source's owned vals (not all vals in shared container) - Works correctly with shared containers **Copy constructor:** - Shares container pointer with source (no new container created) - Registers with shared container - Delegates to static copy method to clone nodes **Move assignment:** - Added self-assignment check - Added deprecation notes for `IrContainer::swap` and `IrContainer::copy` - Added per_fusion_vals_/per_fusion_exprs_ swapping - Updated `inContainer` to check `sharing_fusions_` instead of `parent_` Added 23 new tests for copy/move semantics: - Task 5 (Copy): CopySharesContainer, CopyRegistersWithContainer, CopiedNodesOwnedByNewFusion, CopyOwnedValsAreIndependent, etc. - Task 6 (Move): MoveConstructorTransfersOwnership, MoveUpdatesStatementOwnership, SwapExchangesContainers, SwapUpdatesStatementOwnership, etc. - Phase 2 Container Tests: 49/49 PASSED - Smoke Tests: 34/34 PASSED
This commit fixes two issues discovered during Phase 2 shared container
implementation:
1. Deterministic Accessors Not Filtering by Ownership
- deterministic_vals(), deterministic_exprs(), and their map variants
were returning ALL statements in the shared container instead of
only those owned by the calling Fusion
- Added deterministicValsOwnedBy(Fusion*) etc. to IrContainer
- Updated Fusion methods to use filtered versions
- Map variants now use local indices (0,1,2...) per Fusion
2. StatementGuard Incompatible with Shared Containers
- removeStatementsCreatedAfter used per-Fusion counts but operated on
container-level deques, causing incorrect removal
- Also failed to update per_fusion_vals_/per_fusion_exprs_ tracking
- Now takes Fusion* parameter and only removes owned statements
- Properly updates per-Fusion tracking when removing
Tests added:
- 12 tests for deterministic accessor filtering
- 2 tests for StatementGuard with shared containers
Total Phase 2 tests: 63 passing
Smoke tests: 34 passing
0c3409d to
4bdd3a6
Compare
Replace global IrContainer name counters with per-Fusion counters so cloned Fusions produce matching statement names (T0=T0, T1=T1) instead of incrementing names (T0=T10). This fixes cross-fusion name lookups in GreedyParams and normalization_utils which use tv->name() as map keys. Changes: - Add per_fusion_val_name_map_ and per_fusion_expr_name_counter_ to IrContainer - Update getValName/getExprName to use per-Fusion counter with global fallback - Update registerVal/registerExpr to pass owning Fusion to name generators - Handle counter lifecycle in swap, copy, clear, destroy, transferOwnership - Use deterministic_vals() in Fusion::copy for stable clone ordering - Add 8 new tests for name correspondence (71/71 Phase 2 tests pass)
4bdd3a6 to
56cf217
Compare
|
!test |
No description provided.