Aligned splits, take 2. - #9409
Conversation
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Gemini Pro 3.1 <gemini@aistudio.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… those blend operations in case of aligned splits. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Fix old copy-paste bug in simplifier rules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rename split_aligned_2d_6x6.cpp to split_aligned_2d_3x3.cpp and shrink the pattern to 3x3, which reproduces the surviving mux with a much smaller amount of IR to read. Also fix the test itself: realize the 3-D output with a 3-D shape, check all three channels, sweep all nine (offset_x, offset_y) alignments, and include c in the reorder so it stays innermost. With c left outermost it was unrolled around the xo/yo nest, triplicating the loop nest and recomputing R/G/B once per channel. The test currently fails at the mux count (27 = 9 tile positions x 3 channels); the runtime results are correct for every alignment.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9409 +/- ##
==========================================
+ Coverage 69.95% 69.99% +0.04%
==========================================
Files 261 261
Lines 79402 79493 +91
Branches 19360 19379 +19
==========================================
+ Hits 55546 55644 +98
- Misses 17932 17938 +6
+ Partials 5924 5911 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A loop of eight whose first and last iterations are special and whose interior is periodic with period two. Unrolling the interior by two folds the % away, but only if the unrolled pairs line up with the periodicity, which means the tiles have to start where the interior does. An aligned split says exactly that, and partitioning then peels one iteration at each end rather than two, leaving a steady-state loop of three rather than two. Checks the extent of the remaining loop, that the modulo folded away, and the values. Dropping the alignment from the split fails the extent check, so the test is measuring the thing it claims to.
…IONS The Makefile build (used by CI's Makefile job) defaults WITH_EXCEPTIONS to unset, so libHalide reports errors via abort() instead of throwing. The unguarded try/catch around the intentionally-failing compute_at schedule never got a chance to catch anything in that configuration, so the process aborted instead of exercising the demonstration. Every other place in the tree that expects a Halide::Error already guards this the same way (see test/error/*.cpp).
- <stdio.h> -> <cstdio> (modernize-deprecated-headers) - main() no longer takes unused argc/argv (misc-unused-parameters) - Explicit `protected:` on the visit() overrides in Counter and FindProducer, matching IRVisitor's own visibility instead of the implicit private (misc-override-with-different-visibility) - .extents[0] -> .extents.at(0) (cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) - Nested ternaries replaced with immediately-invoked if/else lambdas (readability-avoid-nested-conditional-operator) - reserve(9) before the two 9-element ways.push_back() loops (performance-inefficient-vector-operation)
| } else { | ||
| // Legacy: structurally guaranteed to be >= old_min | ||
| guarded = promise_clamped(old_var, old_var, old_max); | ||
| } |
There was a problem hiding this comment.
The original comment explaining why promise_clamped is necessary seems to have been removed. Also, imo would be simpler as:
Expr guarded = promise_clamped(old_var, split.align.defined() ? old_min : old_var, old_max);
There was a problem hiding this comment.
Restored the comment. I'll leave the if in place and the cases as separate as I like the style of comments better.
| mask = select(base == old_base, likely(const_true()), mask); | ||
| Expr mask; | ||
| if (split.align.defined()) { | ||
| // Because base is anchored to align instead of old_min, the |
There was a problem hiding this comment.
I think we have a nested tail strategy tail that tries lots of things in combination. It would be good to add aligned splits to it to get more coverage of this.
There was a problem hiding this comment.
I have two tests added: split_aligned_nested and rfactor_split_aligned_nested which do this. I'm a bit hesitant to conflate the existing nested_tail_strategies with another axis of tests.
|
|
||
| /** A version of split() that additionally takes a runtime-valued | ||
| * phase, 'align', which need not be known at compile time. An | ||
| * ordinary split() tiles a Var starting at its own loop_min, so |
There was a problem hiding this comment.
This is user-facing, and users don't know what a loop_min is. I think this should be reworded to say it aligns the splits to absolute coordinates instead of the start of Halide's inferred loop bounds.
There was a problem hiding this comment.
Rewrote a big part of this. Please re-read. 😃
| // partitioning builds (a lower-bound comparison ANDed with an | ||
| // upper-bound comparison, both against the same stride, e.g. | ||
| // (0 <= ramp(b0, s, n)) && (ramp(b1, s, n) <= extent)) | ||
| rewrite(h_and((broadcast(x, arg_lanes) <= ramp(y, z, arg_lanes)) && |
There was a problem hiding this comment.
Would this be simpler?
rewrite(h_and(broadcast(x, arg_lanes) <= ramp(y, z, arg_lanes) && w, 1),
x <= y + min(z * (arg_lanes - 1) && h_and(w, 1), 0))
I.e. the same as the rule above, but with a confounding && w inside the h_and. Then the same for ramp <= broadcast. This would simplify a few more cases (like an && of three things).
There was a problem hiding this comment.
Seems like you made a mistake with the parenthesis: the , 0) of the min( is behind the && h_and(). But yeah, I think that's a good improvement.
There was a problem hiding this comment.
Actually, thinking about this... While it does make sense, these are not simplification rules in the strict sense of less IR nodes. I agree that those are simplifications mathematically: less math in the final result due to reduced lane count. However, if the rules above are considered fine, than this peeling-variant of this rule should also be fine, by the transitivity of being-fine. 😂
There was a problem hiding this comment.
Fewer IR nodes is one term in a lexicographically ordered list of what makes something count as "simpler". Turning a vector computation into a scalar computation is earlier in the list. See https://github.com/halide/Halide/blob/main/apps/simplifier_rule_verifier/reduction_order.cpp#L389
| rewrite(h_or(broadcast(x, arg_lanes) < ramp(y, z, arg_lanes), 1), | ||
| x < y + max(z * (arg_lanes - 1), 0)) || | ||
| rewrite(h_or(broadcast(x, arg_lanes) < ramp(y, z, arg_lanes), 1), | ||
| rewrite(h_or(broadcast(x, arg_lanes) <= ramp(y, z, arg_lanes), 1), |
There was a problem hiding this comment.
Yes, Claude highlighted this as being a copy-paste error from the same rule above (but with < instead of <=). I guess we (that is Claude and I) ran into this during testing and Claude figured this out. It sure looks like a bug to me.
| #include <iostream> | ||
|
|
||
| using namespace Halide; | ||
| using namespace Halide::Internal; |
There was a problem hiding this comment.
This is a little problematic as a tutorial - it reaches into Halide::Internal, which tutorials shouldn't do, and it talks about things internal to Halide's lowering, like "loop_min". I think it might be better as a correctness test.
There was a problem hiding this comment.
I actually really like seeing IR as part of a tutorial. The two visitors are useful as a tutoring element to be able to show the muxes in the IR and automatically count them. The auto-counted muxes are perhaps irrelevant once the IR is printed: the user can see for themselves. What do you think? Should we move find_producer to somewhere more interesting as part of some of the common tutorial utilities?
There was a problem hiding this comment.
Not something to litigate here, I don't think. Drop the tutorial on this branch and we can discuss whether and how to teach users about the IR / lowering internals later.
There was a problem hiding this comment.
Removed the tutorial.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Andrew Adams <andrew.b.adams@gmail.com>
Inner loops go from 0 to factor, to help with constant bounds analysis.
2D tiled test: compute_at test overwrites the compute and storage bounds by just passing those in the schedule.
It's now possible to align the first iteration of the inner loop, like so:
Replaces #9371
Breaking changes
Checklist