De-duplicate variable rename and free-variable collecting mutators - #9364
Draft
alexreinking wants to merge 2 commits into
Draft
De-duplicate variable rename and free-variable collecting mutators#9364alexreinking wants to merge 2 commits into
alexreinking wants to merge 2 commits into
Conversation
substitute() only rewrites Variable/Let/LetStmt/For, so several call sites that needed to rename Allocate/Free/Load/Store identifiers (or apply a deterministic prefix/rename policy to a whole subtree) each hand-rolled their own small IRMutator. Add Renamer<Policy>/rename_ir() in Rename.h, a template (not std::function-boxed, matching the LambdaMutator/mutate_with idiom in IRMutator.h) that applies a memoized name -> name policy across every name-bearing IR node. Migrate the six call sites whose renaming is a flat, injective function of the name (no shadowing elimination needed): Qualify.cpp, ParallelRVar.cpp's RenameFreeVars, Simplify.cpp's can_prove debug canonicalizer, FuseGPUThreadLoops.cpp's register allocation renamer, and both of CodeGen_D3D12Compute_Dev.cpp's renamers. UniquifyVariableNames.cpp is intentionally left alone: its job is to eliminate shadowing (force simultaneously-live same-named bindings to diverge), which needs per-binding-occurrence scope tracking that a pure name->name policy can't express. The D3D12 shared-allocation renamer keeps its original per-Allocate- occurrence unique_name() call (via a small mutate_with lambda) rather than a single whole-Stmt rename_ir call, since two distinct shared allocations can legitimately share an original name (e.g. GuardWithIf-duplicated boundary branches), and HLSL requires globally unique top-level declaration names even across mutually-exclusive branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add FreeVariables (src/FreeVariables.h/.cpp), a Scope-aware IRVisitor that collects the free variables of an Expr/Stmt subtree, mirroring the accumulator style of Closure::include(). Replace three near- identical private reimplementations of the same algorithm with it: UniquifyVariableNames.cpp's FindFreeVars, Simplify.cpp's can_prove debug helper, and SimplifyCorrelatedDifferences.cpp's TrackFreeVars. Also add UnboundVarChecker, a graph-aware sibling (IRGraphVisitor, since these validate raw user-authored condition Exprs that may share subexpressions) used to check whether an Expr references a free Var/RVar and, optionally, calls a Halide Func. RDom.cpp's CheckRDomBounds already did this correctly with its own Scope tracking; Func.cpp's CheckForFreeVars (guarding Stage::specialize) and Pipeline.cpp's Checker (guarding Pipeline::add_requirement) did not track Let-shadowing at all, so they rejected any condition containing so much as a self-contained Let, since they flag every non-Param/Image Variable node unconditionally. Both are migrated to UnboundVarChecker, fixing the bug. A check_func_calls flag preserves each site's prior func-call-checking behavior exactly (RDom.cpp and Pipeline.cpp checked for it; Func.cpp did not and still doesn't). Pipeline.cpp's Variable check previously exempted only Param-backed variables, not Image-backed ones; standardized to exempt both, matching the other two sites. Add test/correctness/unbound_var_checker_let_shadowing.cpp, verified to fail against the pre-fix logic before being added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
alexreinking
added
the
code_cleanup
alexreinking
marked this pull request as draft
August 19, 2026 18:54
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9364 +/- ##
==========================================
+ Coverage 69.97% 70.18% +0.20%
==========================================
Files 259 261 +2
Lines 79158 79095 -63
Branches 19293 19286 -7
==========================================
+ Hits 55394 55512 +118
+ Misses 17898 17805 -93
+ Partials 5866 5778 -88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
Can delete Qualify. There's a real issue in D3D12. Add headers to Makefiles |
Member
|
I suggest deleting Qualify.cpp/h entirely |
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
2 participants
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.
We have a lot of random copies of mutators that rename a symbol or collect free variables. This PR aims to consolidate them.
Breaking changes
None
Checklist