gemini-code-assist[bot] commented on code in PR #19584:
URL: https://github.com/apache/tvm/pull/19584#discussion_r3258490910
##########
src/arith/transitive_comparison_analyzer.cc:
##########
@@ -566,20 +619,40 @@ void TransitiveComparisonAnalyzer::Impl::Bind(const
tirx::Var& var, const Range&
TVM_FFI_ICHECK(allow_override) << "Binding of variable " << var << " as
" << range
<< " conflicts with previous binding as "
<< (*it).second;
if (auto key = ExprToPreviousKey(var)) {
- knowns_.erase(std::remove_if(knowns_.begin(), knowns_.end(),
- [&](const auto& known) { return
known.lhs_ == key.value(); }),
- knowns_.end());
+ 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;
Review Comment:
The vector `to_remove` is currently a copy of `idx_it->second`. Since
self-comparisons (where `partner_key == old_key`) are explicitly skipped and we
are not modifying the map's structure during iteration, using a const reference
would avoid an unnecessary copy and allocation. This is particularly beneficial
if a key has many associated comparisons.
```suggestion
const std::vector<Comparison>& to_remove = idx_it->second;
```
##########
src/arith/transitive_comparison_analyzer.cc:
##########
@@ -566,20 +619,40 @@ void TransitiveComparisonAnalyzer::Impl::Bind(const
tirx::Var& var, const Range&
TVM_FFI_ICHECK(allow_override) << "Binding of variable " << var << " as
" << range
<< " conflicts with previous binding as "
<< (*it).second;
if (auto key = ExprToPreviousKey(var)) {
- knowns_.erase(std::remove_if(knowns_.begin(), knowns_.end(),
- [&](const auto& known) { return
known.lhs_ == key.value(); }),
- knowns_.end());
+ 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);
}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
