[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.
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.