Allow the simplifier to use facts in its can_prove() predicates. - #9400
mcourteaux wants to merge 57 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9400 +/- ##
==========================================
+ Coverage 70.86% 70.99% +0.13%
==========================================
Files 261 261
Lines 79989 80459 +470
Branches 19494 19642 +148
==========================================
+ Hits 56687 57125 +438
- Misses 17470 17492 +22
- Partials 5832 5842 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Was this the one that inflated the lowering time of lens_blur? Is this superseded by your approach in aligned splits take 2? |
|
Some more data: yesterday in a research branch I came across a case where max(x, y) was not simplifying inside an if (x <= y) branch, and it was causing wmma ops to fail to be extracted. This is a case we need to handle, we just need to figure out how to do it without increasing compile times. |
|
I think an approach to make this fast might be to define an operator< that can compare IRMatcher patterns to Exprs, so that the pattern can be looked up in the set of known facts without constructing an IR node, rather than needing to build an Expr just to do the lookup. |
This indeed did slow down lens blur by 10% more ore less. It's not superseded: take 2 just works around the simplification issue by using .bound_extent() and .bound_storage().
🤝
That sounds like a decent approach! Feel free to take over this branch! |
|
What if instead, we special handle the generic-form As such, no We can keep the more expensive machinery for non-trivial rules, such as the ones now in Simplify_Div, which wouldn't trigger for every Max/Min node, because the LHS of the rewrite rule is more specific: has_facts() &&
(rewrite(max(x * c0, y) / c0, x, c0 > 0 && known_true(x >= y / c0, this)) ||
rewrite(max(y, x * c0) / c0, x, c0 > 0 && known_true(x >= y / c0, this)) ||
rewrite(min(x * c0, y) / c0, x, c0 > 0 && known_true(x <= y / c0, this)) ||
rewrite(min(y, x * c0) / c0, x, c0 > 0 && known_true(x <= y / c0, this)))The existing Later, when we |
|
Offline discussion with @abadams, with some ideas going back and forth, Andrew proposed to:
As such, we can have a function that return rewrite(max(x, y), x, min_diff(x, y, this) >= 0) // because x - y >= 0, we know that x >= y. |
The condition of a can_prove predicate in a rewrite rule was simplified on its own, without any of the facts the simplifier has learned on the way down the IR. Substitute those facts into the condition first, and store facts in the same comparison direction the simplifier produces, so that a fact stated as x > y is usable when it visits y < x. This makes fact-driven rewrite rules possible: max/min now pick a side when the facts order the operands, and a division can cancel a multiplication inside a max or min. Co-authored-by: Claude <noreply@anthropic.com>
Facts and the conditions of can_prove predicates are now looked up in the same canonical form: GT and GE are mapped onto LT, Not is unwrapped, and a comparison can be settled by the other strictness of the same comparison in either direction. This means it no longer matters how a fact was spelled relative to how the rule that consumes it was, and a strict fact such as x > y settles the non-strict predicate the max/min rules ask for. Those rules ask non-strictly, since a tie makes either side of a max or min an equally good answer, so a fact of x >= y is enough to pick a side. Co-authored-by: Claude <noreply@anthropic.com>
Simplifying the condition of a can_prove predicate visits the operands again, so a fact-driven rule that matches every node of its type recursed without bound on nested min/max trees. Disable those rules while inside a can_prove condition; the facts themselves are still substituted in at every level. Co-authored-by: Claude <noreply@anthropic.com>
Recursing further is occasionally useful in principle, but measurably expensive: at a limit of 2, correctness_likely goes from 1.0s to 4.2s and correctness_autodiff from 3.4s to 11.4s, with no test producing a better simplification. Keep the limit at one level, but name the constant. Co-authored-by: Claude <noreply@anthropic.com>
can_prove as a rewrite predicate recursively invokes the simplifier on every expression matching the rule's left-hand side, so a rule whose left-hand side also matches something built while proving the predicate recurses. It is also simply expensive. known_true instead looks the condition up in the facts directly. It cannot recurse, and it is cheap enough to use on a rule that matches every node of its type. The fact-driven max, min and division rules now use it, which is enough for all of them: looking up a comparison already understands direction and strictness. Co-authored-by: Claude <noreply@anthropic.com>
The depth limit was checked in has_facts, which only protects rules that consult it. Checking it on entry to the condition simplification instead protects every can_prove, including the pre-existing rules and any future one, and returning the condition unsimplified is the natural way to decline: the predicate simply fails to prove anything. That also frees has_facts to be a plain check, so the non-recursive known_true rules can fire at any depth. The limit is raised to four, which restricts nothing today: instrumenting every correctness test shows the deepest can_prove nesting any of them reaches is one. Co-authored-by: Claude <noreply@anthropic.com>
Refusing to simplify the condition past the depth limit meant the predicate could never be proven there, even when the fact needed was already known. substitute_facts is a plain tree walk (mutate_with over the generic IRMutator base traversal) that never invokes a rewrite rule, so it cannot re-trigger can_prove or known_true and stays safe at any depth: use it as the fallback instead of returning the condition untouched. Added a regression test built on the pre-existing can_prove-based min/max subtraction cancellations in Simplify_Sub.cpp (the rules that motivated the depth limit in the first place, since their predicate constructs a fresh subtraction that can itself match the same rule). With the limit disabled it hangs (confirmed: 15s timeout); with it in place it completes in under a second. Co-authored-by: Claude <noreply@anthropic.com>
The previous fallback ran substitute_facts, a full tree walk, on the condition. But the only thing the caller checks is whether the result is literally the constant true, and nothing runs afterward to fold a compound expression: an And of two individually-known-true operands stays an unfolded And, never becoming true. So substitute_facts's ability to resolve facts about pieces of a compound condition was wasted work here — it can't prove anything is_known_true on the condition itself couldn't already, since folding that partial progress into a verdict is exactly the recursive work the cap exists to avoid. Co-authored-by: Claude <noreply@anthropic.com>
known_true had to build the comparison it was asked about, so a rule like rewrite(max(x, y), a, known_true(y <= x, this)) allocated on every max node with a fact in scope -- and lookup_fact allocated a few more internally while canonicalizing. Measured on a nest of 200 max/min nodes with one fact, that was several allocations per node. Instead, learn a ConstantInterval on the difference between the two sides of each comparison, and ask about it with the operands a rule already has bound. MatcherState holds raw node pointers, so the query touches no reference counts and builds nothing: the same benchmark now allocates nothing per node. Direction and strictness stop being special cases: the other direction is the negated interval, and strictness is just whether the bound is -1 or 0. The complement of a half-line is a half-line, so only the negation of an equality fails to be an interval, and that is always a single point removed, which is what KnownBound::invert represents. A removed point tightens the bounds when it lands on an end, and is otherwise only tracked when it is at zero, which is what decides known_not_equal. Constant offsets are peeled off both the facts and the queries, so a fact about x and y + 3 settles a question about x and y. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
The limit governs how much work an adversarial expression can provoke, and the growth is steep: on a nest of min(x, y) - min(z, w) the simplify test costs 0.02s at a limit of 1 or 2, 0.11s at 3 and 0.72s at 4. Nothing needs the extra depth -- instrumenting every correctness test shows the deepest nesting any of them reaches is one -- and correctness_likely and correctness_autodiff are unchanged across limits of 1, 2, 4 and 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
Two constants, and a min or max compared against one of its own operands, bound their difference on their own. Deriving those needs no facts, no recursion and no allocation -- a node type check and a couple of the inlined equal() comparisons -- so fold them in alongside what the fact table says rather than treating facts as the only source of knowledge. The fact table being empty must no longer short-circuit the whole query, since that would skip these too. No rule needs this yet: the max and min rules that consume min_diff are already covered for these shapes by dedicated rewrite rules, so this changes no behaviour on its own. It is what makes the difference helpers strong enough to replace can_prove in rules that currently rely on it proving things structurally, which without this loses cancellations such as min(x, y) - min(x, w) where y is min(a, b) and w is a. Cost is confined to a synthetic max/min chain (0.070 to 0.079 ms on a 200-deep nest); correctness_likely and correctness_autodiff are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
A min is at most either of its operands and a max is at least either, which bounds their difference on one side without any facts. Knowing the two are unequal removes the endpoint of that bound, and the two together decide a comparison that neither decides alone -- which is what makes these reachable through the max and min rules, where the shapes that structural knowledge settles on its own are already covered by dedicated rewrite rules. The two negative cases pin that down: drop the inequality and the difference could still be zero, drop the shape and there is no bound to tighten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
The fact list is not short in practice. Lowering lens_blur performs 35594 difference lookups, about two thirds of them with 39 to 54 facts in scope, and not one of them matches: every lookup scanned the whole list, following two pointers per record, to establish nothing. That scan was most of what the fact-driven max and min rules cost. Summarize each side of a record by its node type, plus the name or value of the leaves that distinguish otherwise identical nodes. Equal Exprs always summarize alike, so a mismatched summary rules a record out without touching the Exprs, and the scan becomes a pass over integers stored in the record itself. Measured on lens_blur lowering in retired instructions, which wall time is far too noisy to resolve: 2.187G on main, 2.297G before this change, 2.218G after, so it removes about seventy percent of the overhead. Of what remains, 18M is the rules being attempted on every max and min at all, and only 13M is the scan -- so an associative container in place of the vector could recover at most a further half percent, while costing the O(1) scope teardown that truncating a vector gives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
has_facts is true whenever anything at all has been learned, but a fact only leaves a record for min_diff and max_diff to find if it is a comparison of non-overflowing integers. A boolean fact, or one about a type that can wrap, satisfies has_facts while leaving the difference table empty, so the max and min rules were running lookups that could not possibly match. Lowering lens_blur did that 6998 times, a fifth of all its difference lookups. They scanned nothing -- there was nothing to scan -- but still paid for the call, the constant peeling and the structural check. Gating on the table the predicates actually read removes them: 35594 lookups become 28596, with the records scanned unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
Xoring the two fingerprints gives a key that is the same whichever way round the pair is asked about, so a single bit serves both directions of a record. Keeping a bit per key over the whole table turns the common answer -- that nothing is known about this pair -- into one test instead of a walk. The summary belongs to the table rather than to each record: the fallback scan walks every record, so keeping those small matters more than where the summary lives, and a scope can then save and restore it wholesale, which is what makes undoing it free when bits cannot be cleared one at a time. Four words rather than one because a table of a few dozen facts saturates 64 bits and lets four queries in ten through; at 256 it rejects 79.5% of them. Lowering lens_blur, in retired instructions against 2.187G on main: 2.216G before, 2.210G at 64 bits, 2.208G at 256. Skipping the scan entirely would be 2.205G, so what remains of it is 3M instructions, or 0.14%. An associative container cannot do better than not looking at all, so that is the whole of what one could still win here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
…e bit Only leaves carry anything that tells two nodes of the same type apart, so every Add summarizes alike, as does every Min. Xoring a pair of them therefore gives zero whatever the type, and Add against Add, Min against Min and every other same-type pair shared a single bit of the table summary. Keying that case by the kind instead lifts rejection on lens_blur from 79.5% to 81.6% for the cost of one comparison, and the summary is no sparser for it: 32.8 bits of 256 either way. Two larger changes were tried first and both measured worse. Summarizing an Expr recursively rather than only at its root costs more to compute than the scan it saves (2.212G against 2.208G). Replacing the xor with a key built from the sum as well spreads same-type pairs properly but aligns query keys with record keys far more often, dropping rejection to 52.3%. The scan that is left is 3M instructions, so there was never much here to win. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
032f724 to
260cc16
Compare
hannk's average and max pooling clamp the index they read the input at, and then restrict the reduction domain with a predicate that says the same thing: that the index is within the input. Learning a bound on the difference from that predicate let the max and min rules drop the clamp, which is true of the value but not of the region: bounds inference only partly models the conditions of ifs, so it went on to ask for a region the clamp had been keeping in range, and the pipeline failed its own bounds check -- input is accessed at 0, which is before the min (1) in dimension 1. A clamp around an index is load-bearing for more than its value, so only record differences from sources whose ranges bounds inference derives the same way we do: loop bounds, and assumptions the caller states outright. The lowered IR for every hannk generator matches main again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
Suppressing those facts outright, as the previous commit did, fixed hannk by making the feature inert: lowering lens_blur learned 663 differences and used none of them. The condition of an if is the richest source of orderings there is, and loop partitioning, which produces most of them, runs long after the regions are settled. What matters is not where a fact came from but when it is used. Until lowering has finished reading regions and allocation sizes out of the IR, a clamp around an index is part of how those are derived and must not be removed on the strength of a condition; afterwards those regions are IR of their own and a redundant clamp is only a redundant clamp. So gate on that instead, at the one point that decides it: don't learn the difference, rather than remembering it and hoping every consumer checks. A future consumer of known_difference cannot get this wrong, and nothing pays to build a table that may not be read. Lowering lens_blur now learns 2704 differences and settles 522 comparisons with them, and every hannk generator still lowers to what main does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1YKwTyubRmLMfA58gM1Pu
|
@abadams Ready for review. Original post updated. This was all Claude with a lot of guidance, so perhaps a lot of comments are still too verbose, and maybe a few names left and right could be better. However, I think the approach is good. Performance impact of the lookups was real, so we iterated a bit to make them even faster using a cheap hashing scheme. This was the initial histogram of number of facts present, during fact lookup happening within lens_blur:
The massive amount of lookups when there were no facts was fixed after this chart was made. Claude argued by doing some analysis on different runs of retired instruction count that a std::map would not make things faster; not actually measured yet. |
| // friends. When nothing is known the fold reports overflow, which the rewriter | ||
| // already treats as a failed predicate, so the rule simply doesn't fire. | ||
| template<typename A, typename B, typename Prover, bool is_min> | ||
| struct DiffBound { |
There was a problem hiding this comment.
Is it necessary to have this and ScaledDiffBound? Can't ScaledDiffBound just represent these cases? The helpers min_diff and max_diff could remain
There was a problem hiding this comment.
Renamed to LinearDiffBound, and deleted the other.
| // Every pass that reads a region or an allocation size out of the IR has | ||
| // now run, so from here a clamp is only worth what its value is worth, and | ||
| // the simplifier may use what it knows to remove a redundant one. | ||
| ScopedRegionsInferred regions_inferred; |
There was a problem hiding this comment.
This is very unfortunate. What precisely breaks without this intentional reduction in simplifier strength? Can those passes just instead leverage the simplifier, e.g. by inheriting from it?
There was a problem hiding this comment.
BoundsInference does not take an injected if from a RDom where-clause into account when the index Expr is a compound Expr of 2+ variables (e.g., x*stride + r.x). The injected if-guard from the where clause gets rewritten to a solved Expr for the innermost loop variable, which causes the bound on the compound statement to be rewritten from x*stride + r.x < upper to x < (upper - r.x) / stride, which is now no longer picked up as a bound on the compound Expr.
The hannk generator was aware of this limitation, and introduces a redundant clamp, which is supposed to help out bounds inference see (i.e., just give it) the bound of this compound index Expr.
Now, because the new simplifier strength, the redundant clamp is now simplified away rightfully, because it's is sitting within the if-guard of the where clause.
I 100% agree that ScopedRegionsInferred is not the right solution to this problem.
There was a problem hiding this comment.
@abadams I think the solution Fable found is really neat: NameGuardedIndices. Whenever a compound index-Expr is encountered in an enclosing if-guard, it injects a LetStmt replacing the compound Expr with a new temporary. This way, the index-Expr is again a single variable, and the existing solve-machinery now works on the new single-Variable.
| if (ca == 0 && cb == 0) { | ||
| return false; | ||
| } | ||
| int64_t g = gcd(ca, cb); |
There was a problem hiding this comment.
If this shows up as hot it the profiler, it may be worth a fast path for powers of two
| internal_assert(d != 0); | ||
|
|
||
| auto round_up = [=](int64_t v) { | ||
| int64_t q = v / d, r = v % d; |
There was a problem hiding this comment.
Can d be negative? Generally we use div_imp and mod_imp in logic like this because of rounding direction issues
There was a problem hiding this comment.
Good to know about these helpers. Reimplemented using those.
| return; | ||
| } | ||
|
|
||
| if (invert) { |
There was a problem hiding this comment.
I think this is for cases where you learn x != y? Is there a real payoff from supporting these? I wonder if it's worth it.
There was a problem hiding this comment.
Well, I thought about adding those, as we tend to often see tail conditions like
if (out.extent.0 % 10 != 0) {
// some tail
}This is not a good reason, as that's just the use case for RoundUp, but at least an inequality does appear.
Not sure if we will ever learn inequalities. Seemed like a cheap enough addition, so I added it.
| } | ||
|
|
||
| void Simplify::ScopedFact::learn_false(const Expr &fact) { | ||
| // Canonicalize the direction of comparisons, so that facts are stored in |
There was a problem hiding this comment.
Hasn't everything passed to learn_true/learn_false already been mutated? This should be dead code
There was a problem hiding this comment.
It seems like you're right. It is dead code from within the compiler. However, the simplifier correctness test passes assumptions directly to the simplifier, which are not in their canonical order. I added a call to simplify() for every assumption in the check_with_assumptions helper of the tests.
The canonicalization is replaced by an internal_assert.
| // Unlike known_true this only looks facts up, never building an Expr | ||
| // and so never recursing back into the simplifier. | ||
| (no_overflow(op->type) && has_facts() && | ||
| (rewrite(max(x * c0, y) / c0, x, c0 > 0 && scaled_min_diff(x, c0, y, 1, this) >= fold(1 - c0)) || |
There was a problem hiding this comment.
Would it be cleaner to put "this" in the IRMatcher state?
There was a problem hiding this comment.
I propose a separate PR for this, as there are many can_prove() rules I'd like to not touch in this PR.
|
#9448 may make the lowering time cost look better, by taming bgu |
|
I made a bunch of relatively minor comments, and I have one larger concern - intentionally limiting the performance of the simplifier before a certain point in lowering feels it should not be necessary, and hints at a deeper problem with making the simplifier more powerful - other passes can no longer keep up. Perhaps those other passes could leverage or be built on top of the simplifier instead. |
LinearDiffBound bounds the linear combination (ca * a - cb * b).
min_diff/max_diff are now shorthands for it with unit coefficients, and
scaled_{min,max}_diff are renamed linear_{min,max}_diff.
known_affine_difference is renamed known_linear_difference to match.
KnownTrue had no remaining users and is removed.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
Every step of peeling a division is already overflow-checked, and the error interval saturates rather than wraps, so the cap only stopped large divisors from matching. Also loop in peel_affine_term with continue/break instead of a progress flag. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
Halide division is Euclidean, so div_imp already rounds the right way for one sign of d and the other only needs a correction when the division is inexact. The inexactness test is done in uint64_t so that it wraps instead of overflowing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
-INT64_MIN doesn't fit, so the quotient isn't representable. div_imp wraps it back to INT64_MIN, which turned a vacuous fact into a bogus upper bound and let the simplifier prove false comparisons. Leave that end of the interval open instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
Every caller in the compiler learns from conditions the simplifier has already visited, and it never produces > or >=, so the branches that canonicalized them were dead. Assert instead. The simplifier test now simplifies its assumptions first, and the INT64_MIN / -1 regression test goes through the else branch of an if, as a real pipeline would. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
A max pool in hannk's style reads its input through a clamp and restricts the reduction domain with a where clause saying the same thing. Facts learned from that clause must not drop the clamp before bounds inference has used it, or the pipeline asks for input it doesn't need and fails its own bounds check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
The window samples every other input pixel, and the where clause bounds 2 * r.x rather than the index itself. Bounds inference solves such a condition for r.x, which divides by the coefficient and loses how r.x relates to the rest of the index. Once the simplifier removes the clamp as redundant, that makes the region of the input too large and the pipeline fails its bounds check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
Bounds inference bounds an index by the condition of an enclosing if one variable at a time, by solving the condition for it. That loses how the variables relate: from min_x <= x*s + 2*r it only learns a bound on r alone, with x at the far end of its range. A clamp around the index used to hide this, but the simplifier can now remove that clamp as redundant with the condition. Before bounds inference visits the IR, bind each compound index (or part of one) that an if condition also mentions to a let, so the condition restricts the let directly and the existing let-bound trimming bounds the index. With that, the region is right without the clamp, so the simplifier no longer has to wait for regions to be inferred before it learns from facts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sS947n7W4KyJG31RCgMZY
Replacing the condition with one on the let-bound index left BoxesTouched nothing to bound the index's own variables by, so a second read at just one of them lost the bound the condition gave it. Nest the named condition inside the original one instead, so that both are used. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
…where clause Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
…able A query walked every record in known_bounds, reading the hash through each record's Expr. A pipeline's image checks alone put hundreds of facts in scope for its whole body, which saturated the summary bitmask and made the walk the single hottest function when lowering resnet_50: 17% of its time, +25% overall. Chaining the records per bucket of their pair key, with the hashes kept in the record, brings that to +5%. Popping a scope now unchains its records one by one rather than truncating, which also makes a scope that ends after an enclosing one, as the assumptions of the public simplify() do, pop only what is still its own. Before, a second assumption that recorded a difference tripped an internal assert. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
Look at the bucket head before reducing the coefficients, return before undoing the canonicalization when nothing was found, and don't take a gcd for a unit coefficient. Nearly every query from the Add/Sub bounds finds nothing, and paid for all three. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
The two rewrite rules each queried the facts, so every min and max paid for two lookups. A direct check before the rules asks once and answers for both sides, and hands back the surviving side's own bounds rather than the union. The Add and Sub bounds likewise skip the intersection when the lookup found nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9fWe3P96CsN75gXDTJYqL

Problem
Rewrite rules can only use what the simplifier knows through
can_prove(cond, this), which builds a fresh Expr and recursively simplifies it. That ignores the learned facts (max(x, y)doesn't fold underif (y <= x)), and it recurses without bound when the rule's LHS matches something built while proving its own predicate:apps/lens_blurdid not finish lowering (killed after 52 CPU-minutes).What this PR does
Facts become bounds on differences. Every comparison the simplifier learns (
ifconditions, asserts,require, loop bounds) is stored as aConstantIntervalonca * a - cb * b:a < bgives<= -1,!(a < b)gives>= 0,a == bgives0,a != bremoves the point0. Constant add/mul/div terms are peeled off both sides and the coefficients reduced to a coprime pair, sox < y / 8,8 * x < y, and3 * x - 6 * yvs2 * x - 4 * yall meet at one record. A bitmask over hashed pairs rejects most lookups before any record is read.Rules read the facts without building IR.
min_diff(x, y, this)/max_diff(...)andlinear_min_diff(x, c0, y, c1, this)/linear_max_diff(...)are rewrite predicates that inspect the nodes a rule has already bound (wildcards only, enforced bystatic_assert). Users:min/maxpick a side when the facts order the operands (Simplify_Min.cpp,Simplify_Max.cpp).max(x * c0, y) / c0 -> xand theminvariant (Simplify_Div.cpp).Add/Subintersect with the known difference, somax(x - 5 * y, 0)folds from5 * y < xthrough the ordinary bounds machinery, with no rule per shape.can_provegets a depth limit (max_can_prove_depth = 2), checked where the recursion happens so it covers existing rules. A regression test insimplify.cpphangs without it.Facts must be simplified before they are learned (
learn_true/learn_falseassert on>/>=), so they are stored in the form the simplifier produces when it meets them. Theassumptionsargument ofsimplify()inherits this requirement.Bounds inference:
NameGuardedIndices(src/Bounds.cpp)Why it's there. With facts, the simplifier removes a clamp that an enclosing
ifmakes redundant. hannk's max pool readsinput(clamp(x * s + r.x, min_x, max_x))underwhere(min_x <= x * s + r.x && ...). Bounds inference derives the input's required region from the IR after the first simplification (image checks, allocation bounds, storage folding), andBoxesTouchedonly uses anifcondition by solving it for one variable at a time. It can't boundx * s + r.xas a whole, so once the clamp was gone the region asked ofinputgrew to whatever the unclamped index could reach, and the pipeline failed its own bounds check.What it does. Inside
boxes_touched, every compound index in anif's body that the condition also mentions gets a name, and the condition is repeated in terms of it:becomes
Why it works.
tis a let, soBoxesTouchedhas its bounds in scope and the inner condition solves for it like any variable, givingf's index exactly the bound the clamp used to give. The original condition stays outside so the component variables are still trimmed for every other index they appear in (the dependent-let machinery recomputestfrom the trimmed components, so the two combine). The rewrite is applied to the copyboxes_touchedtakes by value and never reaches the lowered IR. Lowering time is unchanged within noise (+0.8% over 18 generators).Tests:
bounds_of_compound_index_from_where.cppcovers the hannk shape, a scaled and a halved index, and two cases where a second input is read at only part of the named index.Compile-time impact
Lowering only (
-e stmt, wall clock, 5 reps, machine otherwise idle), 75apps/generators, this branch with main (d82d5a3) merged, against that main: +2.0% in total (9510 ms vs 9704 ms; sum of per-generator minimums also +2.0%).local_laplacian+11% andstencil_chain+6% are the largest costs;dgemv-25% andsgemv-14% go the other way, from loop trimming. Summed over all generators the cost sits in the simplifier-driven passes (second simplification +17%, partitioning loops +15%, removing dead allocations +12%, finding intrinsics +10%, vectorizing +11%), where everyAdd,Sub,minandmaxinside a fact scope now asks the fact table once.The fact table is chained per bucket of the pair's hashes, so a query reads only the records that can be about its pair. A pipeline's image checks alone leave hundreds of facts in scope for its whole body, and a linear scan over them was the single hottest function when lowering
resnet_50(+25%, for byte-identical output); it is now +2%. The full per-generator table and pass breakdown:Lowered IR: 39 of 75 pipelines are byte-identical, 3 differ only in temporary numbering, 33 change. Where operator counts move, they go down:
dgemv-15max-75min,sgemv-17max-63min,sgemm/dgemm-11max-13min-11/each,hannk/DepthwiseConv-6if,halide_blur-2if-4min,fft-9max. Inlocal_laplacian(-32max, -44min) andinterpolate(-21max, -6min) the foldedmax/minwere the pyramid-level bounds, each amin/maxof two affine chains over one variable (min(gPyramid1.s0.v1.min*2 - 1, output.min.1)). Once decided, every level collapses to a single chain like((output.min.1 - 127)/128)*4, and the simplifier's let peeling then substitutes such a chain at each use instead of keeping the let, so the text gains/(+105 and +38) while getting shorter overall. LLVM folds most of that back (29 moresar, +0.4% text in the object), and run time is unchanged:local_laplacianat 2048x1536, 8 levels, is 24.0-24.7 ms on main and 23.6-24.3 ms here.Behaviour change
max(x * 8, y) / 8now folds toxgiveny / 8 <= x.simplify.cpppreviously asserted it stays put; that was a limitation, not a truth.Checklist
🤖 Generated with Claude Code