[ARITH] Index TransitiveComparisonAnalyzer knowns by Key#19584
[ARITH] Index TransitiveComparisonAnalyzer knowns by Key#19584gigiblender wants to merge 1 commit into
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the TransitiveComparisonAnalyzer by replacing the linear knowns_ vector with a map-based index (knowns_by_key_), allowing for more efficient lookups during comparison analysis. It also refactors constraint addition into persistent and scoped methods and adds a comprehensive test suite. Review feedback identified a deficiency in the index cleanup logic within the Bind method, specifically regarding cases where an overridden variable appears as the right-hand side of a comparison. Furthermore, a suggestion was made to improve the test for bind overrides to ensure it correctly verifies the removal of old constraints when ranges do not overlap.
There was a problem hiding this comment.
Code Review
This pull request optimizes the TransitiveComparisonAnalyzer by replacing the linear search of known comparisons with a per-key index using an unordered_map. This change significantly improves the efficiency of constraint lookups and transitive proof searches. The feedback focuses on strengthening the new test suite, specifically by ensuring that binding overrides are verified with non-overlapping ranges and that the cross-key lookup test correctly exercises the new indexed storage rather than the scoped vector.
gigiblender
force-pushed
the
upstream/arith-knowns-by-key
branch
2 times, most recently
from
edcb7fd to
e2685c5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes the TransitiveComparisonAnalyzer by introducing a per-key index for known comparisons, replacing the previous linear search. The implementation refactors AddKnown and EnterConstraint to distinguish between persistent and scoped facts and updates Bind and search methods to utilize the new indexing structure. Feedback suggests further optimization in Bind to prevent redundant entries when bindings remain unchanged and recommends using a const reference during index cleanup to avoid unnecessary vector copies.
| Key old_key = key.value(); | ||
|
|
||
| // Every entry in `knowns_by_key_[old_key]` involves old_key by | ||
| // construction (on either side). Remove each from its partner | ||
| // bucket and then drop the whole old_key bucket in one go. | ||
| // Snapshot the entries first because the source and partner | ||
| // buckets may overlap. | ||
| auto idx_it = knowns_by_key_.find(old_key); | ||
| if (idx_it != knowns_by_key_.end()) { | ||
| std::vector<Comparison> to_remove = idx_it->second; | ||
| for (const auto& cmp : to_remove) { | ||
| Key partner_key = (cmp.lhs_ == old_key) ? cmp.rhs_ : cmp.lhs_; | ||
| // self-comparison (lhs_ == rhs_): only stored once, in | ||
| // the bucket we are about to erase. | ||
| if (partner_key == old_key) continue; | ||
| auto other_it = knowns_by_key_.find(partner_key); | ||
| if (other_it == knowns_by_key_.end()) continue; | ||
| other_it->second.erase( | ||
| std::remove(other_it->second.begin(), other_it->second.end(), cmp), | ||
| other_it->second.end()); | ||
| } | ||
| knowns_by_key_.erase(idx_it); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| prev_bindings_.Set(var, range); | ||
|
|
||
| if (is_const_int(range->extent, 1)) { | ||
| AddKnown(var == range->min, &knowns_); | ||
| AddKnown(var == range->min); | ||
| } else { | ||
| AddKnown(var >= range->min, &knowns_); | ||
| AddKnown(var < range->min + range->extent, &knowns_); | ||
| AddKnown(var >= range->min); | ||
| AddKnown(var < range->min + range->extent); | ||
| } |
There was a problem hiding this comment.
The current implementation of Bind (inherited from the previous version) has a logic issue where it calls AddKnown even when the range hasn't changed (i.e., when differs_from_previous is false). This leads to redundant entries being pushed into the knowns_by_key_ vectors every time Bind is called with the same range.
While the cleanup logic handles this by removing all matching comparisons when a variable is finally overridden, the accumulation of duplicates can degrade performance and increase memory usage during the lifetime of the analyzer.
Consider wrapping the prev_bindings_.Set and AddKnown calls in a check that ensures they only run if the binding is new or has actually changed.
gigiblender
force-pushed
the
upstream/arith-knowns-by-key
branch
from
e2685c5 to
67e976c
Compare
CollectDirectComparisons and DFSFromLHS previously linear-scanned the unbounded `knowns_` vector on every TryCompare call. When Simplify walks many For nodes, this is O(N^2) in the number of For visits and produces an order-of-magnitude slowdown on large IR. Replace `knowns_` with `knowns_by_key_`, a map keyed by Key. Each Comparison is stored under both its keys (or once when they match). CollectDirectComparisons walks only the smaller of the two query-key buckets; DFSFromLHS walks only the bucket for the current expansion key. scoped_knowns_ stays a flat vector because its scope-exit truncation would require extra bookkeeping to keep in sync. Add Comparison::operator== for index cleanup on Bind override, split AddKnown into AddKnown / AddScopedKnown, and add tests/python/arith/test_arith_transitive_comparison.py.
gigiblender
force-pushed
the
upstream/arith-knowns-by-key
branch
from
67e976c to
0101c0f
Compare
1 participant
CollectDirectComparisonsandDFSFromLHSpreviously linear-scanned the unboundedknowns_vector on everyTryComparecall. WhenSimplifywalks many For nodes, this is O(N^2) in the number of For visits and produces an order-of-magnitude slowdown on large IR.Replace
knowns_withknowns_by_key_, a map keyed by Key. Each Comparison is stored under both its keys (or once when they match). CollectDirectComparisons walks only the smaller of the two query-key buckets; DFSFromLHS walks only the bucket for the current expansion key. scoped_knowns_ stays a flat vector because its scope-exit truncation would require extra bookkeeping to keep in sync.Add
Comparison::operator==for index cleanup on Bind override, split AddKnown into AddKnown / AddScopedKnown, and addtests/python/arith/test_arith_transitive_comparison.py.