REFACTOR: tiler reads VariableBuffer.alias_of instead of legacy _alias - #203
REFACTOR: tiler reads VariableBuffer.alias_of instead of legacy _alias#203mert-ogul wants to merge 1 commit into
Conversation
Wire TilerExtension, MemoryScheduler, and dealiasBuffer to directed alias_of so Reshape no longer needs the _alias workaround. Fixes pulp-platform#201.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces legacy ChangesDirected alias ancestry
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Deeploy/DeeployTypes.py`:
- Around line 570-576: Define a canonical storage-root resolution rule across
all alias consumers: in Deeploy/DeeployTypes.py lines 570-576, update the
alias-resolution logic to traverse all parent paths and accept only one
canonical root or reject conflicting roots; in
Deeploy/TilingExtension/MemoryScheduler.py lines 658-661, use that canonical
parent or verify all eligible parents share its address; in
Deeploy/TilingExtension/TilerExtension.py lines 337-345, resolve the parent
selected by the current pattern rather than choosing an unrelated lexicographic
parent; and in DeeployTest/testTypes.py lines 227-252, add coverage where the
in-pattern parent is not lexicographically first.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4e067bc-d689-49fe-8eb4-ad2fda7b0fce
📒 Files selected for processing (8)
CHANGELOG.mdDeeploy/DeeployTypes.pyDeeploy/Targets/Generic/Templates/ReshapeTemplate.pyDeeploy/Targets/PULPOpen/Templates/ReshapeTemplate.pyDeeploy/TilingExtension/MemoryScheduler.pyDeeploy/TilingExtension/TilerExtension.pyDeeployTest/testSchedulingExtension.pyDeeployTest/testTypes.py
💤 Files with no reviewable changes (1)
- Deeploy/Targets/PULPOpen/Templates/ReshapeTemplate.py
| while alias.alias_of: | ||
| seenAliases.add(alias.name) | ||
| alias = self.lookup(alias._alias) | ||
| # Reshape and other current aliasers have a single storage parent. | ||
| # Pick a deterministic parent if multiple are ever present. | ||
| parentName = sorted(alias.alias_of)[0] | ||
| alias = self.lookup(parentName) | ||
| assert isinstance(alias, VariableBuffer) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define one valid storage resolution rule for multi-parent aliases.
alias_of now permits multiple parents, but these consumers select a parent by name order. This can assign an arbitrary address. In Deeploy/TilingExtension/TilerExtension.py, an in-pattern parent can satisfy line 340 while dealiasBuffer() selects another root that is absent from nodeList; line 345 then fails.
Deeploy/DeeployTypes.py#L570-L576: resolve all parent paths and require one canonical storage root, or reject conflicting roots.Deeploy/TilingExtension/MemoryScheduler.py#L658-L661: use the canonical storage parent or verify that all eligible parents have the same address.Deeploy/TilingExtension/TilerExtension.py#L337-L345: resolve the parent selected from the current pattern instead of an unrelated lexicographic parent.DeeployTest/testTypes.py#L227-L252: add a multi-parent case where the in-pattern parent is not the lexicographically first parent.
📍 Affects 4 files
Deeploy/DeeployTypes.py#L570-L576(this comment)Deeploy/TilingExtension/MemoryScheduler.py#L658-L661Deeploy/TilingExtension/TilerExtension.py#L337-L345DeeployTest/testTypes.py#L227-L252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Deeploy/DeeployTypes.py` around lines 570 - 576, Define a canonical
storage-root resolution rule across all alias consumers: in
Deeploy/DeeployTypes.py lines 570-576, update the alias-resolution logic to
traverse all parent paths and accept only one canonical root or reject
conflicting roots; in Deeploy/TilingExtension/MemoryScheduler.py lines 658-661,
use that canonical parent or verify all eligible parents share its address; in
Deeploy/TilingExtension/TilerExtension.py lines 337-345, resolve the parent
selected by the current pattern rather than choosing an unrelated lexicographic
parent; and in DeeployTest/testTypes.py lines 227-252, add coverage where the
in-pattern parent is not lexicographically first.
There was a problem hiding this comment.
I think this is a valid comment, can you please take a look?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Relevant file locations =='
fd -t f '^(DeeployTypes|MemoryScheduler|TilerExtension|testTypes)\.py$' .
printf '%s\n' '== Alias-related definitions, writes, and reads =='
rg -n -C 4 --glob '*.py' \
'(?:\.alias_of\b|alias_of\s*=|dealiasBuffer\s*\(|aliases\.(?:add|update)\s*\()' \
Deeploy DeeployTest
printf '%s\n' '== TilerExtension outline =='
ast-grep outline Deeploy/TilingExtension/TilerExtension.py --items all --type function --match '.*alias.*|.*Memory.*|.*convert.*' || true
printf '%s\n' '== MemoryScheduler outline =='
ast-grep outline Deeploy/TilingExtension/MemoryScheduler.py --items all --type function --match '.*alias.*|.*Memory.*|.*solution.*' || true
printf '%s\n' '== Current sibling ranges =='
sed -n '320,360p' Deeploy/TilingExtension/TilerExtension.py
sed -n '640,675p' Deeploy/TilingExtension/MemoryScheduler.py
sed -n '210,270p' DeeployTest/testTypes.pyLength of output: 21435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== All direct alias_of mutations =='
rg -n -C 3 --glob '*.py' \
'(?:\.alias_of\.(?:add|update|clear|remove|discard)\s*\(|\.alias_of\s*=)' \
.
printf '%s\n' '== aliasedBlocks use sites and scheduling continuation =='
rg -n -C 6 '\baliasedBlocks\b' Deeploy/TilingExtension/MemoryScheduler.py
sed -n '620,760p' Deeploy/TilingExtension/MemoryScheduler.py
printf '%s\n' '== Static schedule method context =='
sed -n '263,365p' Deeploy/TilingExtension/TilerExtension.py
printf '%s\n' '== Existing tests that construct MemoryBlock patterns =='
rg -n -C 3 --glob 'test*.py' \
'(?:MemoryBlock\s*\(|_convertCtxtToStaticSchedule\s*\(|getBlockVector\s*\(|scheduleMemoryConstraints\s*\()' \
DeeployTestLength of output: 12040
@Xeratec, I confirmed that the finding is valid.
VariableBuffer.alias_of is a Set[str], and the PR objective requires multi-alias support. However, NetworkContext.dealiasBuffer() selects sorted(alias.alias_of)[0]. This selection does not define a storage rule when parent paths reach different roots.
There is a concrete failure in Deeploy/TilingExtension/TilerExtension.py. The code enters the alias branch when any parent is in blockNames. It then calls dealiasBuffer(). If the lexicographically first parent is outside the pattern, aliasNodes is empty and the assertion fails.
MemoryScheduler.py has a related issue. It assigns the address space from a selected immediate in-pattern parent. However, its global-buffer shortcut first uses the root returned by dealiasBuffer(). That result can be unrelated to the in-pattern parent.
The current built-in reshape template adds only one parent. Therefore, current reshape generation does not expose the defect. This does not invalidate the finding because the new representation and PR scope permit multiple parents.
I recommend keeping the finding open and changing the implementation to do one of these actions:
- Require all
alias_ofpaths to resolve to the same storage root, and reject conflicting roots. - Define an explicit selected-parent rule and use that same parent in
dealiasBuffer(),MemoryScheduler, andTilerExtension.
The test should add two parents where the in-pattern parent is not lexicographically first. It should verify either correct allocation from the selected storage root or a clear rejection of conflicting roots.
You are interacting with an AI system.
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I only have two small comments. Please check out the comment from CodeRabbitAI and extend the "List of Pull Requests" in the CHANGELOG.md
| ## Unreleased (Planned Release Target: v0.2.2) | ||
|
|
||
|
|
||
| ### List of Pull Requests |
There was a problem hiding this comment.
Please also add this PR to the "List of Pull Requests" in the CHANGELOG.md
| while alias.alias_of: | ||
| seenAliases.add(alias.name) | ||
| alias = self.lookup(alias._alias) | ||
| # Reshape and other current aliasers have a single storage parent. | ||
| # Pick a deterministic parent if multiple are ever present. | ||
| parentName = sorted(alias.alias_of)[0] | ||
| alias = self.lookup(parentName) | ||
| assert isinstance(alias, VariableBuffer) |
There was a problem hiding this comment.
I think this is a valid comment, can you please take a look?
3 participants
Wire TilerExtension, MemoryScheduler, and dealiasBuffer to directed alias_of so Reshape no longer needs the _alias workaround. Fixes #201 (duplicate of #130).
Describe the intent of your PR here.
Added
VariableBuffer.alias_of: Set[str]for directed storage-parent edges used by tiling.testDealiasBufferUsesAliasOfinDeeployTest/testTypes.py.Changed
NetworkContext.dealiasBuffer,TilerExtension, andMemorySchedulerreadalias_ofinstead of legacy_alias.ReshapeTemplatesetsbufferOut.alias_of(keeps bidirectionalaliasesfor liveness).testSchedulingExtension.pyusesalias_of.CHANGELOG.mdupdated under Unreleased.Fixed
_aliasattribute that templates had to set as a workaround.Removed
_aliasworkaround from Generic/PULPOpenReshapeTemplate.PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.