-
Notifications
You must be signed in to change notification settings - Fork 84
PR #5961 Review Fixes — Per-Fusion Statement Tracking - #6015
Merged
mdavis36 merged 4 commits intoFeb 26, 2026
Merged
Conversation
Replace manual iterate-and-erase loops in removeStatementsOwnedBy with std::erase_if, which does single-pass partition-and-truncate instead of repeated O(n) deque shifts. This fixes O(n2) performance when a Fusion with many statements is destroyed.
...ortcuts Change Fusion::numExprs() and Fusion::numVals() to return counts from per_fusion_exprs_/per_fusion_vals_ (filtered to this Fusion) instead of global container counts. This is necessary for correct StatementGuard behavior with shared containers. Add numValsExcludingShortcuts() helper that subtracts active shortcut val pointers (zero_val_, one_val_, etc.) from the per-Fusion count. These singletons should persist across guard scopes, so the count must exclude them for LIFO pop-back consistency in removeStatementsCreatedAfter.
...tainers Update Fusion::removeStatementsCreatedAfter to compare per-Fusion counts (from exprsOwnedBy(this) and numValsExcludingShortcuts()) instead of global deque sizes. This correctly handles shared containers where other Fusions' statements would inflate the global counts. Add NVF_ERROR assertions to verify the LIFO invariant: the tail element of the global deque must belong to this Fusion. If violated, another Fusion appended concurrently (should be prevented by PR #5971 locking). Remove now-unnecessary deque size validation checks.
Change StatementGuard constructor to snapshot numExprs() (already per-Fusion) and numValsExcludingShortcuts() instead of numVals() (now also per-Fusion). This ensures the snapshot is consistent with the LIFO rollback logic in removeStatementsCreatedAfter, which correctly handles shortcut singletons.
mdavis36
commented
Feb 26, 2026
Collaborator
Author
!test
Description
|
| Relevant files | |
|---|---|
| Enhancement |
fusion.cpp
Fix removeStatementsCreatedAfter for per-Fusion counts and LIFO verificationcsrc/fusion.cpp exprsOwnedBy(this) ownership shortcut handling expressions and values container.cpp
Optimize removeStatementsOwnedBy performance with std::erase_ifcsrc/ir/container.cpp performance statements statement_guard.cpp
Update StatementGuard to use per-Fusion counts excluding shortcutscsrc/statement_guard.cpp of numVals() removeStatementsCreatedAfter fusion.h
Convert count methods to per-Fusion and add shortcut exclusion helpercsrc/fusion.h pointers |
PR Reviewer Guide
Here are some key observations to aid the review process:
🧪 PR contains tests
⚡ Recommended focus areas for review
LIFO Invariant Validation
The new NVF_ERROR checks verify that tail elements belong to the correct Fusion before popping. This is crucial for maintaining the LIFO invariant in per-fusion statement tracking. The error messages are clear and provide context for debugging.
c->per_fusion_exprs_[this].count(e) > 0, "removeStatementsCreatedAfter: tail expr belongs to another Fusion");
Performance Optimization
The optimization from manual iterator manipulation to std::erase_if is a good improvement. This changes the complexity from O(n2) to O(n) for the removeStatementsOwnedBy function. The lambda-based approach is cleaner and more maintainable.
std::erase_if(vals_up_, [&](const std::unique_ptr<Val>& v) { if (owned.count(v.get()) > 0) { vals_.erase(v.get()); return true; } return false; });
Shortcut Value Handling
The new numValsExcludingShortcuts() method correctly handles the edge case where shortcut values (zero_val_, one_val_, etc.) should persist across StatementGuard scopes. This ensures LIFO ordering works correctly by excluding these singleton values from the count used for rollback.
int64_t numValsExcludingShortcuts() const noexcept { int64_t count = std::ssize(ir_container()->valsOwnedBy(this)); count -= (zero_val_ != nullptr) + (one_val_ != nullptr) + (true_val_ != nullptr) + (false_val_ != nullptr) + (magic_zero_val_ != nullptr); return count; }
Contributor
Greptile SummaryApplies review fixes from PR #5961 for per-Fusion statement tracking in shared containers:
All changes are well-implemented with defensive assertions to catch violations of the LIFO invariant. The optimizations are algorithmically sound and the per-Fusion counting ensures correct behavior when multiple Fusions share an Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 0abaf11 |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, no comments
mdavis36
added a commit
that referenced
this pull request
Mar 3, 2026
## Summary Review fixes for PR #5961 (Per-Fusion statement tracking): - **O(n2) → O(n)**: Optimize `removeStatementsOwnedBy` with `std::erase_if` - **Per-Fusion counts**: Convert `numExprs()`/`numVals()` to return per-Fusion counts instead of global - **StatementGuard fixes**: Snapshot and compare per-Fusion counts for correct LIFO rollback in shared containers - **LIFO assertions**: Verify tail elements belong to this Fusion before popping ## Tests All tests pass: - ✅ StatementGuardTest.ExecuteAfterGuard - ✅ StatementGuardTest.LazySpecialValsNotDangling - ✅ FusionCopy_CUDA - ✅ FusionMove_CUDA
mdavis36
added a commit
that referenced
this pull request
Mar 4, 2026
## Summary Review fixes for PR #5961 (Per-Fusion statement tracking): - **O(n2) → O(n)**: Optimize `removeStatementsOwnedBy` with `std::erase_if` - **Per-Fusion counts**: Convert `numExprs()`/`numVals()` to return per-Fusion counts instead of global - **StatementGuard fixes**: Snapshot and compare per-Fusion counts for correct LIFO rollback in shared containers - **LIFO assertions**: Verify tail elements belong to this Fusion before popping ## Tests All tests pass: - ✅ StatementGuardTest.ExecuteAfterGuard - ✅ StatementGuardTest.LazySpecialValsNotDangling - ✅ FusionCopy_CUDA - ✅ FusionMove_CUDA
mdavis36
added a commit
that referenced
this pull request
Mar 4, 2026
## Summary Review fixes for PR #5961 (Per-Fusion statement tracking): - **O(n2) → O(n)**: Optimize `removeStatementsOwnedBy` with `std::erase_if` - **Per-Fusion counts**: Convert `numExprs()`/`numVals()` to return per-Fusion counts instead of global - **StatementGuard fixes**: Snapshot and compare per-Fusion counts for correct LIFO rollback in shared containers - **LIFO assertions**: Verify tail elements belong to this Fusion before popping ## Tests All tests pass: - ✅ StatementGuardTest.ExecuteAfterGuard - ✅ StatementGuardTest.LazySpecialValsNotDangling - ✅ FusionCopy_CUDA - ✅ FusionMove_CUDA
mdavis36
added a commit
that referenced
this pull request
Mar 5, 2026
## Summary Review fixes for PR #5961 (Per-Fusion statement tracking): - **O(n2) → O(n)**: Optimize `removeStatementsOwnedBy` with `std::erase_if` - **Per-Fusion counts**: Convert `numExprs()`/`numVals()` to return per-Fusion counts instead of global - **StatementGuard fixes**: Snapshot and compare per-Fusion counts for correct LIFO rollback in shared containers - **LIFO assertions**: Verify tail elements belong to this Fusion before popping ## Tests All tests pass: - ✅ StatementGuardTest.ExecuteAfterGuard - ✅ StatementGuardTest.LazySpecialValsNotDangling - ✅ FusionCopy_CUDA - ✅ FusionMove_CUDA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Review fixes for PR #5961 (Per-Fusion statement tracking):
removeStatementsOwnedBywithstd::erase_ifnumExprs()/numVals()to return per-Fusion counts instead of globalTests
All tests pass: