Solve: treat min/max of booleans as And/Or in SolveForInterval - #9457
Conversation
solve_for_outer_interval / solve_for_inner_interval mishandled min/max of
boolean operands. Boolean AND/OR of comparisons is represented as min/max by
and_condition_over_domain and by the simplifier, but SolveForInterval implemented
visit(And)/visit(Or)/visit(Not)/visit(Select) and not visit(Min)/visit(Max). So a
negated min-of-bools was not distributed via De Morgan; e.g.
solve_for_outer_interval(!min(x < 0, x < 2), "x")
returned [2, +inf) instead of [0, +inf) (min(x<0, x<2) == x<0, so !min(...) is
x>=0).
This caused a silent miscompile: trim_no_ops relaxes a loop's no-op condition
over inner loops with and_condition_over_domain (yielding min/max of comparisons)
and trims the loop via solve_for_outer_interval. The wrong interval trimmed a
serial loop to a single iteration, dropping work -- e.g. a wavefront/diamond-tiled
recurrence with a GuardWithIf (likely) split tail over a non-tile-multiple extent
produced incorrect results under parallelism.
Fix: add visit(Min)/visit(Max) to SolveForInterval, rewriting a boolean min to
And and max to Or (mirroring the existing visit(Select)).
Adds regression checks to test/correctness/solve.cpp.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
Change LGTM but I'm suspicious of the comment claiming that these can come from the simplifier. Doesn't the simplifier normalize these to And/Or? |
@abadams It does not; I had claude check this directly. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9457 +/- ##
==========================================
- Coverage 70.21% 69.98% -0.23%
==========================================
Files 261 261
Lines 79792 79983 +191
Branches 19451 19491 +40
==========================================
- Hits 56022 55973 -49
- Misses 17966 18112 +146
- Partials 5804 5898 +94 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Maybe we shouldn't even have And/Or and just pretty-print min and max on bools differently |
|
@wraith1995 Just to be clear; you'll want to update the comment for this to be merged. |
@mcourteaux I am confused; the comment about the simplifier is accurate because it does indeed not simplify the min/max away. @abadams I am happy to try that- I suspect it can happen with very few changes to the code base. |
|
The PR is valid as-is, but perhaps the simplifier should normalize Min/Max on bools to And/Or (making the comment inaccurate)? Removing And/Or was just me musing about longer term paths. |
4 participants
Fixes #9456
Checklist