Add more let peeling and rewrapping helpers - #9319
Open
abadams wants to merge 3 commits into
Open
Conversation
Peeling Lets or LetStmts into a vector of name/value pairs, doing something to the body, and then rewrapping is a common pattern in the compiler. Rewrapping conditionally was done by calling expr_uses_var on the partially-rebuilt body once per let, which is quadratic. Adds a Stmt overload of peel_lets, plus rewrap_used_lets and rewrap_all_lets for Expr and Stmt. rewrap_used_lets gathers the names the body mentions once and extends the set with the value of each let it keeps, so it is linearithmic. It conservatively treats every name mentioned as a possible reference to a peeled let, even where an inner let shadows it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
IRGraphVisitor memoizes nodes with a refcount above one, so a name removed from the set can't be re-added by a later traversal that reaches it through a shared subexpression. Testing without removing keeps rewrap_used_lets as conservative as its documentation claims. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Holding a vector of const LetStmt * only works while some handle keeps the chain alive, which is an easy thing to get wrong for no measurable gain here. Use the same name/value pairs as the rest of the compiler. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9319 +/- ##
==========================================
+ Coverage 69.83% 69.85% +0.02%
==========================================
Files 258 258
Lines 78210 78165 -45
Branches 19037 19006 -31
==========================================
- Hits 54617 54604 -13
+ Misses 17832 17793 -39
- Partials 5761 5768 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Peeling Lets or LetStmts into a vector of name/value pairs, doing
something to the body, and then rewrapping is a common pattern in the
compiler. Rewrapping conditionally was done by calling expr_uses_var on
the partially-rebuilt body once per let, which is quadratic.
Adds a Stmt overload of peel_lets, plus rewrap_used_lets and
rewrap_all_lets for Expr and Stmt. rewrap_used_lets gathers the names
the body mentions once and extends the set with the value of each let it
keeps, so it is n log(n) instead of n^2. It conservatively treats every name
mentioned as a possible reference to a peeled let, even where an inner
let shadows it.
There should be no functional changes here, but while working on it Claude
noticed that the peeled lets in the store collector in Deinterleave.cpp are
unused. It looks like it was supposed to take the lets by reference instead of
by value, but if you do that you get duplicates in the list. It is currently trying
to figure out if this is just dead code or if this masks a bug.