Change for loops from min/extent to min/max #8858
Open
+356
−453
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now they're just regular old lets placed there for the convenience of computing splits. They don't need to accurately reflect the loop, or be preserved through lowering passes, etc. loop_extent was deleted entirely. They do need to be preserved until computation bounds inference because they provide a way to compute loop bounds as a function of .min .max symbols that don't exist until after that point, so you can't move them around before then. This change was possible because allocation bounds inference doesn't need any assistance given loops that are min/max instead of min/extent.
Also, simplify the loop extents when offloading GPU kernels and add extra outputs in the bgu makefile.
alexreinking
reviewed
Dec 6, 2025
|
|
||
| debug(1) << "Simplifying...\n"; | ||
| s = simplify(s, false); // Storage folding and allocation bounds inference needs .loop_max symbols | ||
| s = simplify(s); |
Member
There was a problem hiding this comment.
Seems like a great opportunity to simplify the simplifier! (In a follow up PR of course)
Co-authored-by: Alex Reinking <[email protected]>
alexreinking
approved these changes
Dec 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an experiment in changing how we represent for loops from min/extent to min/max. In C terms, it's a change from this:
to this:
The reason to do this is that we need the max anyway for bounds inference, so we keep it around awkwardly as a .loop_max symbol, and then need to jump through hoops to preserve it. My thought was maybe we should just represent loops using the max to begin with, because we don't need the extent nearly so much, and we can just compute it on the fly when needed.
The result is a net deletion of code, and up to 2x faster lowering (because we can delete unused lets earlier in lowering).
I'm not comfortable merging this yet, as it's a big change to a core data type, but I wanted to open it as a PR to get buildbot testing to see what's broken.
Edit: I forgot to say the original motivation - sliding window. With a min/max loop you just bump the min in isolation. With a min/extent loop you have to also subtract from the extent, and then hope things simplify later after loop partitioning.
Second Edit: After dev meeting discussion, I think this is a good thing to merge