Skip to content

Change WireBundle::Ctrl to WireBundle::TileControl. Decouple WireBundle and StrmSwPortType #2010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 10, 2025

Conversation

fifield
Copy link
Collaborator

@fifield fifield commented Jan 8, 2025

This changes replaces the abbreviated WireBundle::Ctrl with WireBundle::TileControl and removes an implicit coupling between the WireBundle enum names and the aie-rt StrmSwPortType enum names.

Previously the xaie target (e.g. aie-translate --aie-generate-xaie) used the TableGen'd wireBundleStringify functions to emit StrmSwPortType enums directly as C++ source code. This PR replaces uses of wireBundleStringify in AIETargetXAIEV2 with a new utility function. This corrects a potential bug related to the PL port and allows the renaming of the control port.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@stephenneuendorffer
Copy link
Collaborator

While I'm normally very much in favor of avoiding abbreviations, I'm not 100% on board with this. The WireBundle names come from the architecture and at some point it's not good to have that inconsistency. But I guess there has to be some friction somewhere, the only question is where is the least confusing place for it to be?

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Coverage Report

Created: 2025-01-08 21:36

Click here for information about interpreting this report.

FilenameFunction CoverageLine CoverageRegion CoverageBranch Coverage
include/aie/Targets/AIERT.h 0.00% 0.00% 0.00% -
lib/Dialect/AIE/IR/AIETargetModel.cpp 82.35% 91.96% 92.32% 91.39%
lib/Dialect/AIE/Transforms/AIEGenerateColumnControlOverlay.cpp 70.59% 76.67% 81.94% 77.19%
lib/Dialect/AIE/Transforms/AIEPathFinder.cpp 87.50% 91.35% 88.51% 82.61%
Totals 78.26% 88.18% 89.20% 86.72%
Generated by llvm-cov -- llvm version 14.0.0

@fifield
Copy link
Collaborator Author

fifield commented Jan 8, 2025

While I'm normally very much in favor of avoiding abbreviations, I'm not 100% on board with this. The WireBundle names come from the architecture and at some point it's not good to have that inconsistency. But I guess there has to be some friction somewhere, the only question is where is the least confusing place for it to be?

As far as I know the Ctrl name comes from aie-rt StrmSwPortType enum. I see the port referred to as "tile_control" in the arch spec. Maybe that would be better?

@stephenneuendorffer
Copy link
Collaborator

I think it came from the AIE1 arch spec. TileControl indeed might be better? Given that it is itself inconsistent, I remove my objection :)

@fifield fifield changed the title Change WireBundle::Ctrl to WireBundle::Control. Decouple WireBundle and StrmSwPortType Change WireBundle::Ctrl to WireBundle::TileControl. Decouple WireBundle and StrmSwPortType Jan 8, 2025
@fifield
Copy link
Collaborator Author

fifield commented Jan 8, 2025

I think it came from the AIE1 arch spec. TileControl indeed might be better? Given that it is itself inconsistent, I remove my objection :)

Updated to TileControl to be more....consistent.

fifield and others added 4 commits January 8, 2025 14:16
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Revert unintended find/replace
revert unintended find/replace
@fifield fifield added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit df46f74 Jan 10, 2025
53 checks passed
@fifield fifield deleted the wirebundle_stringify branch January 10, 2025 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants