[Arith] Updated BufferDomainTouched to use IRVisitorWithAnalyzer by Lunderberg · Pull Request #11970 · apache/tvm

Lunderberg · 2022-06-30T16:03:36Z

This PR factors extracts functionality of BufferDomainTouched out into IRVisitorWithAnalyzer and IntSetAnalyzer. It is broken up into the following sequence of changes, each in a separate commit for ease of review.

Allow binding of Var in IntSetAnalyzer, similar to the other four subanalyzers in arith::Analyzer.
Updated IRVisitorWithAnalyzer to mimic IRMutatorWithAnalyzer, to allow subclassing when implementing other visitors.
Moved IRVisitorWithAnalyzer to tvm::arith namespace. This is in keeping with its location in src/arith directory, and further mimics IRMutatorWithAnalyzer
Updated BufferDomainTouched to use IRVisitorWithAnalyzer. This used the earlier changes to allow subclasses of IRVisitorWithAnalyzer, and to expose binding/constraints to IntSetAnalyzer.

All tests of BufferDomainTouched in test_arith_domain_touched.py and all tests of DomainTouchedAccessMap in test_tir_te_extern_primfunc.py are all passing locally.

Lunderberg · 2022-06-30T16:04:43Z

@csullivan This was the PR that I mentioned as a follow-up to your refactoring in #11589 and #11722.

The other four subanalyzers in `arith::Analyzer` can each be provided with variable bindings/constraints that are remembered internally. This adds the same capability to `IntSetAnalyzer`, rather than requiring users to independently track and maintain a `Map<Var, IntSet>` containing the domain of each variable, and applies bindings/constraints alongside the other subanalyzers.

Previously, `IRVisitorWithAnalyzer` did not allow subclassing, and could only be used to collect bounds of variables along an entire statement, and could not be used to perform scope-dependent analysis. This commit removes `final` from `IRVisitorWithAnalyzer` and provides the same scope-based constraints/bindings during iteration as are provided by `IRMutatorWithAnalyzer`.

Changing for consistency, since `IRVisitorWithAnalyzer` it is part of the `src/arith` directory and the analogous `IRMutatorWithAnalyzer` is already part of the `arith` namespace.

This used the earlier changes to allow subclasses of `IRVisitorWithAnalyzer`, and to expose binding/constraints to `IntSetAnalyzer`.

Because both sides of a `Select` node are visited regardless of the condition, the `SelectNode::condition` should not be treated as a known value.

Resolves a bug that was found in CI, where an earlier scope-dependent constraint was treated as a conflict by a later global bound.

This way, if a subanalyzer throws an exception during `EnterConstraint`, the other subanalyzers are still appropriately backed out of the constraint.

The `min_value - max_value` in the `CanProveGreaterEqual` argument can result in an exception being thrown for unsigned integers where subtraction would wrap.

Since these are tracked when lowering expressions, should allow post-vectorization expressions. To maintain previous behavior, this only applies when using the automatically tracked `Map<Var, IntSet> dom_map_`. If an explicit domain map is passed, the previous behavior of raising an error for vectorized expressions still occurs.

Previously, the Combine() method didn't handle values without a known lower bound, for boolean operators.

tkonolige

Overall looks good. Thanks @Lunderberg!

To be consistent with other subanalyzers, using "Update" when providing the analyzer with the same data structure as is used internally, and "Bind" used when providing it with something that must be converted to the internal data structure.

tkonolige · 2022-07-13T16:18:04Z

Thanks @Lunderberg!

…ache#11970) * [Arith] Allow binding of Var in IntSetAnalyzer The other four subanalyzers in `arith::Analyzer` can each be provided with variable bindings/constraints that are remembered internally. This adds the same capability to `IntSetAnalyzer`, rather than requiring users to independently track and maintain a `Map<Var, IntSet>` containing the domain of each variable, and applies bindings/constraints alongside the other subanalyzers. * [Arith] Updated IRVisitorWithAnalyzer to mimic IRMutatorWithAnalyzer Previously, `IRVisitorWithAnalyzer` did not allow subclassing, and could only be used to collect bounds of variables along an entire statement, and could not be used to perform scope-dependent analysis. This commit removes `final` from `IRVisitorWithAnalyzer` and provides the same scope-based constraints/bindings during iteration as are provided by `IRMutatorWithAnalyzer`. * [Arith] Moved IRVisitorWithAnalyzer to tvm::arith namespace Changing for consistency, since `IRVisitorWithAnalyzer` it is part of the `src/arith` directory and the analogous `IRMutatorWithAnalyzer` is already part of the `arith` namespace. * [Arith] Updated BufferDomainTouched to use IRVisitorWithAnalyzer This used the earlier changes to allow subclasses of `IRVisitorWithAnalyzer`, and to expose binding/constraints to `IntSetAnalyzer`. * Avoid accidental Bind with dynamic Range * [Arith] Do not visit SelectNode in IRVisitorWithAnalyzer Because both sides of a `Select` node are visited regardless of the condition, the `SelectNode::condition` should not be treated as a known value. * [Arith][IntSet] Track global and scope-dependent bounds separately Resolves a bug that was found in CI, where an earlier scope-dependent constraint was treated as a conflict by a later global bound. * [Arith] Recovery function for each subanalyzer This way, if a subanalyzer throws an exception during `EnterConstraint`, the other subanalyzers are still appropriately backed out of the constraint. * [Arith][IntSet] Use CanProve instead of CanProveGreaterEqual The `min_value - max_value` in the `CanProveGreaterEqual` argument can result in an exception being thrown for unsigned integers where subtraction would wrap. * [Arith] Allow vector expressions in IntSet::operator(PrimExpr) Since these are tracked when lowering expressions, should allow post-vectorization expressions. To maintain previous behavior, this only applies when using the automatically tracked `Map<Var, IntSet> dom_map_`. If an explicit domain map is passed, the previous behavior of raising an error for vectorized expressions still occurs. * Avoid comparisons between integer and handle datatypes * [Arith] IntSet, Combine() extension Previously, the Combine() method didn't handle values without a known lower bound, for boolean operators. * Added docstring * Naming consistency of `IntSetAnalyzer` methods. To be consistent with other subanalyzers, using "Update" when providing the analyzer with the same data structure as is used internally, and "Bind" used when providing it with something that must be converted to the internal data structure.

Follow-up from apache#11970, to improve performance. In the initial implementation, the `analyzer->int_set` would compute the intersection of all scope-based constraints when entering the scope, even if they weren't actually used. This commit delays the call to `Intersect` until required, following the same behavior as `ConstIntBound`.

…12821) Follow-up from #11970, to improve performance. In the initial implementation, the `analyzer->int_set` would compute the intersection of all scope-based constraints when entering the scope, even if they weren't actually used. This commit delays the call to `Intersect` until required, following the same behavior as `ConstIntBound`.

…ache#11970) * [Arith] Allow binding of Var in IntSetAnalyzer The other four subanalyzers in `arith::Analyzer` can each be provided with variable bindings/constraints that are remembered internally. This adds the same capability to `IntSetAnalyzer`, rather than requiring users to independently track and maintain a `Map<Var, IntSet>` containing the domain of each variable, and applies bindings/constraints alongside the other subanalyzers. * [Arith] Updated IRVisitorWithAnalyzer to mimic IRMutatorWithAnalyzer Previously, `IRVisitorWithAnalyzer` did not allow subclassing, and could only be used to collect bounds of variables along an entire statement, and could not be used to perform scope-dependent analysis. This commit removes `final` from `IRVisitorWithAnalyzer` and provides the same scope-based constraints/bindings during iteration as are provided by `IRMutatorWithAnalyzer`. * [Arith] Moved IRVisitorWithAnalyzer to tvm::arith namespace Changing for consistency, since `IRVisitorWithAnalyzer` it is part of the `src/arith` directory and the analogous `IRMutatorWithAnalyzer` is already part of the `arith` namespace. * [Arith] Updated BufferDomainTouched to use IRVisitorWithAnalyzer This used the earlier changes to allow subclasses of `IRVisitorWithAnalyzer`, and to expose binding/constraints to `IntSetAnalyzer`. * Avoid accidental Bind with dynamic Range * [Arith] Do not visit SelectNode in IRVisitorWithAnalyzer Because both sides of a `Select` node are visited regardless of the condition, the `SelectNode::condition` should not be treated as a known value. * [Arith][IntSet] Track global and scope-dependent bounds separately Resolves a bug that was found in CI, where an earlier scope-dependent constraint was treated as a conflict by a later global bound. * [Arith] Recovery function for each subanalyzer This way, if a subanalyzer throws an exception during `EnterConstraint`, the other subanalyzers are still appropriately backed out of the constraint. * [Arith][IntSet] Use CanProve instead of CanProveGreaterEqual The `min_value - max_value` in the `CanProveGreaterEqual` argument can result in an exception being thrown for unsigned integers where subtraction would wrap. * [Arith] Allow vector expressions in IntSet::operator(PrimExpr) Since these are tracked when lowering expressions, should allow post-vectorization expressions. To maintain previous behavior, this only applies when using the automatically tracked `Map<Var, IntSet> dom_map_`. If an explicit domain map is passed, the previous behavior of raising an error for vectorized expressions still occurs. * Avoid comparisons between integer and handle datatypes * [Arith] IntSet, Combine() extension Previously, the Combine() method didn't handle values without a known lower bound, for boolean operators. * Added docstring * Naming consistency of `IntSetAnalyzer` methods. To be consistent with other subanalyzers, using "Update" when providing the analyzer with the same data structure as is used internally, and "Bind" used when providing it with something that must be converted to the internal data structure.

…pache#12821) Follow-up from apache#11970, to improve performance. In the initial implementation, the `analyzer->int_set` would compute the intersection of all scope-based constraints when entering the scope, even if they weren't actually used. This commit delays the call to `Intersect` until required, following the same behavior as `ConstIntBound`.

…ache#11970) * [Arith] Allow binding of Var in IntSetAnalyzer The other four subanalyzers in `arith::Analyzer` can each be provided with variable bindings/constraints that are remembered internally. This adds the same capability to `IntSetAnalyzer`, rather than requiring users to independently track and maintain a `Map<Var, IntSet>` containing the domain of each variable, and applies bindings/constraints alongside the other subanalyzers. * [Arith] Updated IRVisitorWithAnalyzer to mimic IRMutatorWithAnalyzer Previously, `IRVisitorWithAnalyzer` did not allow subclassing, and could only be used to collect bounds of variables along an entire statement, and could not be used to perform scope-dependent analysis. This commit removes `final` from `IRVisitorWithAnalyzer` and provides the same scope-based constraints/bindings during iteration as are provided by `IRMutatorWithAnalyzer`. * [Arith] Moved IRVisitorWithAnalyzer to tvm::arith namespace Changing for consistency, since `IRVisitorWithAnalyzer` it is part of the `src/arith` directory and the analogous `IRMutatorWithAnalyzer` is already part of the `arith` namespace. * [Arith] Updated BufferDomainTouched to use IRVisitorWithAnalyzer This used the earlier changes to allow subclasses of `IRVisitorWithAnalyzer`, and to expose binding/constraints to `IntSetAnalyzer`. * Avoid accidental Bind with dynamic Range * [Arith] Do not visit SelectNode in IRVisitorWithAnalyzer Because both sides of a `Select` node are visited regardless of the condition, the `SelectNode::condition` should not be treated as a known value. * [Arith][IntSet] Track global and scope-dependent bounds separately Resolves a bug that was found in CI, where an earlier scope-dependent constraint was treated as a conflict by a later global bound. * [Arith] Recovery function for each subanalyzer This way, if a subanalyzer throws an exception during `EnterConstraint`, the other subanalyzers are still appropriately backed out of the constraint. * [Arith][IntSet] Use CanProve instead of CanProveGreaterEqual The `min_value - max_value` in the `CanProveGreaterEqual` argument can result in an exception being thrown for unsigned integers where subtraction would wrap. * [Arith] Allow vector expressions in IntSet::operator(PrimExpr) Since these are tracked when lowering expressions, should allow post-vectorization expressions. To maintain previous behavior, this only applies when using the automatically tracked `Map<Var, IntSet> dom_map_`. If an explicit domain map is passed, the previous behavior of raising an error for vectorized expressions still occurs. * Avoid comparisons between integer and handle datatypes * [Arith] IntSet, Combine() extension Previously, the Combine() method didn't handle values without a known lower bound, for boolean operators. * Added docstring * Naming consistency of `IntSetAnalyzer` methods. To be consistent with other subanalyzers, using "Update" when providing the analyzer with the same data structure as is used internally, and "Bind" used when providing it with something that must be converted to the internal data structure.

Lunderberg · 2022-06-30T16:03:36Z

This PR factors extracts functionality of BufferDomainTouched out into IRVisitorWithAnalyzer and IntSetAnalyzer. It is broken up into the following sequence of changes, each in a separate commit for ease of review.

Allow binding of Var in IntSetAnalyzer, similar to the other four subanalyzers in arith::Analyzer.
Updated IRVisitorWithAnalyzer to mimic IRMutatorWithAnalyzer, to allow subclassing when implementing other visitors.
Moved IRVisitorWithAnalyzer to tvm::arith namespace. This is in keeping with its location in src/arith directory, and further mimics IRMutatorWithAnalyzer
Updated BufferDomainTouched to use IRVisitorWithAnalyzer. This used the earlier changes to allow subclasses of IRVisitorWithAnalyzer, and to expose binding/constraints to IntSetAnalyzer.

All tests of BufferDomainTouched in test_arith_domain_touched.py and all tests of DomainTouchedAccessMap in test_tir_te_extern_primfunc.py are all passing locally.

Lunderberg · 2022-06-30T16:04:43Z