-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Standardize Tile Instantiation and Attachment #2504
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
92cd111
tile: BaseTile HasLogicalTreeNode and RocketTile overrides
hcook 0d4cf59
Tiles are instantiated in HasTiles via TilesLocated Field
hcook 9e79525
om: make tree root require more verbose
hcook 6646b5d
subsystem: RocketTileAttachParams can rely on inherited bound for Til…
hcook 3a593ad
subsystem: more HasTiles comments
hcook 66d56c5
subsystem: always add a WW to tile slave ports, usually passthru
hcook 49d1363
config: MaxHartIdBits derived from TilesLocated
hcook 615b58e
subsystem: always instantiate tiles in the order of their static hartids
hcook 6e8d387
groundtest: update to use HasTiles and TilesLocated
hcook 76ff01f
config: MaxHartIdBits fix for pow2 max id
hcook a340cd3
config: make WithN<>Cores pattern composable
hcook 0286604
subsystem: allow tiles to select notification arity
hcook a43a2af
subsystem: dont drop buffers arg to TileSlavePortParams
hcook ece6b14
InstantiableTileParams -> InstantiableTileParams
hcook 77532ca
config: use optional override offsets
hcook File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be an example for what direct manipulation of the TilesLocated pattern looks like? (Not using the LegacyTileFieldHelper)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up doing this example in
WithTraceGen
for theTraceGenTile
subtype. I'm not very keen to mess too much with the existingRocketTilesKey
Config
alterations since it it seems hard to change them while remaining backwards compatibly with external alterations ofRocketTilesKey
. Check outWithTraceGen
andHeterogeneousTileExampleConfig
and let me know if that is sufficiently helpful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is pretty clear.
I noticed that manipulation of core parameters is more verbose without direct access through
CoreTilesKey
, since the type of the core must be matched, and sinceAttachParams
adds another layer of case-class nesting. Given that manipulating tile-level parameters is the common use-case of the core-specificTilesKey
s, it may be worth defining a generic macro to modify theTileParams
matching somedesired
Tile type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you can figure out a good pattern for applying a "lens" to nested case classes in Config alterations I will buy you a beer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes something specific to this particular use case is probably also possible