[Arith] Implement statistics counters for RewriteSimplifier #14532
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
|
I expect the modified tests of |
Previously, so long as `RewriteSimplifier` produces the same output, unit tests of its behavior would pass. This could have severe performance regressions, such as the one resolved in apache#14528, which caused the runtime of two test to increase from ~1.5 seconds to ~10 minutes each. This commit implements statistics counts in RewriteSimplifier, which are exposed through both the C++ and Python APIs, and uses these to guard against the known performance regression from apache#14528.
Lunderberg
force-pushed
the
rewrite_simplifier_stats_counters
branch
from
083eedf to
b3b75ee
Compare
src/arith/rewrite_simplify.h
Outdated
| struct RewriteSimplifierStatsNode : Object { | ||
| int nodes_visited{0}; | ||
| int constraints_entered{0}; | ||
| int rewrites_attempted{0}; |
There was a problem hiding this comment.
Thank you, updated here and in the SetMaximumRewriteSteps method.
|
One thing that worth thinking about is the relation of simplification and persistence of analyzer. Since we encourage persistence of analyzer in general within a pass, so we should document in max step to encourage users to keep that in mind |
|
Good point on the re-use of the same |
|
|
||
| auto* n = f.CopyOnWrite(); | ||
| n->body = NoOpRemover::Apply(std::move(n->body), &analyzer, std::move(touch_pattern), nullptr); | ||
| { |
There was a problem hiding this comment.
Is there a specific reason for adding curly braces here?
There was a problem hiding this comment.
They aren't necessary for correctness here, but were added present to limit the scope of write_ptr. It doesn't have much impact in a function of this size, especially with the return statement just afterward, but for longer functions it can help to limit the scope of variables that are only needed for a few following lines.
That said, the implementation no longer requires updating the signature of NoOpRemover::Apply, so this change has become unrelated to the overall PR.
4 participants
Previously, so long as
RewriteSimplifierproduces the same output, unit tests of its behavior would pass. This could have severe performance regressions, such as the one resolved in #14528, which caused the runtime of two test to increase from ~1.5 seconds to ~10 minutes each.This commit implements statistics counts in RewriteSimplifier, which are exposed through both the C++ and Python APIs, and uses these to guard against the known performance regression from #14528.