refactor(ir): use pointer identity instead of name_hint_ in IR transforms#624
Conversation
…orms Migrate dependency_analyzer, scope_outline_utils, expand_mixed_kernel, and convert_to_ssa transforms from string-based Var::name_hint_ lookups to const Var* pointer identity. This eliminates reliance on cosmetic naming for semantic correctness. Key changes: - Add CanonicalVarTracker to confine name-based identity in SSA pass - Replace string-keyed maps with pointer-keyed maps across all transforms - Replace canonicalize_name() with pointer-based origin tracing - Add known_names set for SSA name collision avoidance in outliner Fixes hw-native-sys#616
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughSwitches multiple IR transforms and outline utilities to use Var pointer identity ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant refactoring of how variables are identified and tracked within the Intermediate Representation (IR) transformation passes. The core change shifts from using potentially ambiguous string-based name hints to unique pointer identity for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the IR transformation logic to primarily use const Var* pointer identity instead of Var::name_hint_ for variable identification. This significantly enhances the robustness and correctness of the transformations by moving away from fragile string-based comparisons. Key improvements include the introduction of CanonicalVarTracker to centralize canonical variable identity, widespread adoption of const Var* as map keys, and a more robust origin_map for tracing variable derivations in expand_mixed_kernel_pass. The changes are well-aligned with the goal of improving IR stability and correctness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ir/transforms/expand_mixed_kernel_pass.cpp (1)
1170-1182: Consider propagating YieldStmt values to return_vars for completeness.The current implementation propagates
return_var[i] → origin of iter_arg[i](lines 1177-1182), which is correct for loop-carried state. However, the actual carried value comes from the YieldStmt at the end of the loop body, not directly from the iter_arg.This is likely fine in practice because:
- The iter_arg and its yielded value typically share the same origin parameter.
- The goal is just to find an Out/InOut parameter to remap dangling references.
If edge cases arise where a yield value has a different origin than its iter_arg, you may need to also trace through YieldStmt values. For now, this appears sufficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/transforms/expand_mixed_kernel_pass.cpp` around lines 1170 - 1182, The loop currently maps return_vars_ to the origin of iter_args_ (in the ForStmt handling) but doesn't account for YieldStmt values which are the actual carried results; update the ForStmt branch in expand_mixed_kernel_pass.cpp so that after walking origins for the loop body (walk_origins(FlattenBody(for_stmt->body_))) you also scan the body for any YieldStmt nodes, find their yielded expressions and propagate their origins into origin_map for the corresponding return_vars_ (fall back to iter_args_ origin only if a YieldStmt value origin is missing); use the existing helper propagate_from_expr / origin_map lookup logic and reference ForStmt::iter_args_, ForStmt::return_vars_, YieldStmt and FlattenBody to locate and map the correct origins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ir/transforms/expand_mixed_kernel_pass.cpp`:
- Around line 1170-1182: The loop currently maps return_vars_ to the origin of
iter_args_ (in the ForStmt handling) but doesn't account for YieldStmt values
which are the actual carried results; update the ForStmt branch in
expand_mixed_kernel_pass.cpp so that after walking origins for the loop body
(walk_origins(FlattenBody(for_stmt->body_))) you also scan the body for any
YieldStmt nodes, find their yielded expressions and propagate their origins into
origin_map for the corresponding return_vars_ (fall back to iter_args_ origin
only if a YieldStmt value origin is missing); use the existing helper
propagate_from_expr / origin_map lookup logic and reference ForStmt::iter_args_,
ForStmt::return_vars_, YieldStmt and FlattenBody to locate and map the correct
origins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 241922cc-6ced-4abe-9723-81e78f924a2e
📒 Files selected for processing (7)
include/pypto/ir/transforms/utils/scope_outline_utils.hsrc/ir/transforms/convert_to_ssa_pass.cppsrc/ir/transforms/dependency_analyzer.cppsrc/ir/transforms/expand_mixed_kernel_pass.cppsrc/ir/transforms/outline_cluster_scopes_pass.cppsrc/ir/transforms/outline_hierarchy_scopes_pass.cppsrc/ir/transforms/outline_incore_scopes_pass.cpp
There was a problem hiding this comment.
Pull request overview
This PR refactors several IR transforms to stop using Var::name_hint_ (a cosmetic IgnoreField) as semantic variable identity, and instead rely on stable pointer identity (const Var*) for maps, substitution, and dependency tracking.
Changes:
- Refactor outlining passes/utilities to build symbol tables keyed by
const Var*and track used names separately for fresh-name generation. - Update
ConvertToSSAto centralize all name-based “same variable” logic in aCanonicalVarTracker, with the rest of the SSA conversion using canonical pointer keys. - Replace
expand_mixed_kernel_passname-canonicalization with a pointer-based origin map to remap dangling output references.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ir/transforms/outline_incore_scopes_pass.cpp | Switch outlining symbol table seeding to pointer-keyed tables and pass known-name set into outliner. |
| src/ir/transforms/outline_hierarchy_scopes_pass.cpp | Same as above for hierarchy scope outlining. |
| src/ir/transforms/outline_cluster_scopes_pass.cpp | Same as above for cluster scope outlining. |
| src/ir/transforms/expand_mixed_kernel_pass.cpp | Key affinity tracking by const Var* and replace name-suffix canonicalization with a pointer-based origin propagation map. |
| src/ir/transforms/dependency_analyzer.cpp | Switch dependency tracking maps from string keys to const Var* pointer identity. |
| include/pypto/ir/transforms/utils/scope_outline_utils.h | Refactor symbol table collection/outlining helpers to be pointer-keyed and introduce known_names_ for collision-free renaming. |
Add precondition comment to DependencyAnalyzer header noting that input IR must have unique Var pointers per definition (SSA form), since variable identity is now determined by pointer identity. Addresses review comment on hw-native-sys#624
Summary
Var::name_hint_(a cosmeticIgnoreField) as semantic variable identity to usingconst Var*pointer identityCanonicalVarTrackerclass in the SSA converter to confine all name-based identity logic to a single class, with all other SSA logic using canonical pointer keyscanonicalize_name()string-suffix-stripping inexpand_mixed_kernel_passwith a structural pointer-based origin map that traces each Var back to its originating function parameterdependency_analyzer,scope_outline_utils, and all 3 outline pass callersTesting
Related Issues
Fixes #616