-
Notifications
You must be signed in to change notification settings - Fork 33
new components: cam and replacement #259
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
base: main
Are you sure you want to change the base?
Conversation
removed wavedumper
mkorbel1
left a comment
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.
This is wonderful!
lib/src/memory/cam.dart
Outdated
| final int idWidth; | ||
|
|
||
| /// The "enable" bit for this interface, enabling a request. | ||
| Logic get en => port('en'); |
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.
why do we need an enable bit?
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.
To tell it to do the cam match. You are providing a tag in, and the en tells it to do a match and return the idx or data back. This is the same as a DataPortInterface providing an en and addr. Just replace those with en and tag.
You may not want it doing a match if you simply have the tag field set on the read port.
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.
the enable is needed for a write, but if you're doing a read isn't it the same if we just always read whatever address is given? what do you mean "doing a match"? is this for power savings? the combo logic is there either way
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 point and simpler is better.
|
|
||
| /// An interface to a Content-Addressable Memory (CAM) that allows querying | ||
| /// for tags and returns the index of the matching tag if found. | ||
| class TagInterface extends Interface<DataPortGroup> { |
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 i only see it used in one way, and the write ports are DataPortInterfaces. Is there another place you're planning to use them?
lib/src/memory/cam.dart
Outdated
| final int idWidth; | ||
|
|
||
| /// The "enable" bit for this interface, enabling a request. | ||
| Logic get en => port('en'); |
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.
the enable is needed for a write, but if you're doing a read isn't it the same if we just always read whatever address is given? what do you mean "doing a match"? is this for power savings? the combo logic is there either way
| String? logicName, | ||
| }) : super(fields, name: logicName ?? config.name); | ||
|
|
||
| // Historically the CSR disallowed renaming because the architectural |
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.
@kimmeljo any concerns?
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 can't recall if the instance config can override the name from the architectural config of a CSR. If it can, then I don't see why this change would be necessary as the logic name == instance name.
@desmonddak was there a specific reason you needed this?
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.
Now I cannot remember why I needed this. Let me undo and figure out what demanded the name.
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.
these test files are getting pretty long. is there an opportunity to refactor a bit to reduce repetition by making some reusable infra?
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.
The last checkin 104e6aa
you will see a major refactoring of the cache and channel testing.
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.
What's the difference between this file and lib/src/interfaces/request_response_channel.dart?
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 catch. This file is supposed to be the structures and the abstract base class (API).
The other file is supposed to be just a wired passthrough implementation. I accidentally duplicated in the second file.
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.
Two suggestions here for the future (both of which we've discussed):
(1) It might be useful to create a "flavor" of this module that assumes strict ordering of responses relative to requests. Then, instead of a CAM (fully associative cache) to track ordering, we can simplify the HW a bit by using a FIFO. This has the added benefit at the API level of not requiring a unique ID on incoming requests.
(2) It might be useful to create a "flavor" of this module that has a register file instead of a full blown cache on the data side. This would more closely mirror the notion of hash table lookup in HW. The API might have to be different though because it would require explicit invalidation/eviction from the outside.
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.
More of a general question, but does the RegisterFile abstraction allow for use of things like SRAM? I can imagine in some cases we'll need an SRAM at least for the data portion. Maybe we just punt on this for now?
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.
This is an important question. Right now our RegisterFile abstraction supports asynchronous reads and I don't believe SRAM timing will allow that. So this will impact microarchitecture choices because of the additional cycle.
I think TAG side requires multiple ports because the read and write ports need to do a tag match, so I think you are right: we will likely stay with flops on the TAG side, but we need to think about the data side being an actual SRAM.
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.
More of a nitpick, but isn't set associative the general term while fully associative/direct mapped are just special cases (ways=n, 1 respectively). Maybe worth consolidating into a single implementation?
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.
Yes, this is absolutely true. I can try to consolidate, first at the API level, and then perhaps at the implementation level. ROHD is powerful enough to probably make this simple (like single 'way' array probably eliminates some logic signals).
| String? logicName, | ||
| }) : super(fields, name: logicName ?? config.name); | ||
|
|
||
| // Historically the CSR disallowed renaming because the architectural |
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 can't recall if the instance config can override the name from the architectural config of a CSR. If it can, then I don't see why this change would be necessary as the logic name == instance name.
@desmonddak was there a specific reason you needed this?
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.
This is awesome. I vaguely recall in the "regular" FIFO implementation that writeEnable when full and readEnable when empty can cause bad things to happen. I see you've handled that here. Is it worth handling that in the "regular" implementation too? Or am I misremembering?
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 cannot remember if we built the bypasses there as well.
The reason it could be worth handling is things like skidbuffers (single entry FIFOs) that need to be depth 2 if you don't handle these bypass cases. There can be a lot of those that have to have a spare entry.
We need to check this (and make sure there is a clear test for these edge cases to ensure they never break.
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.
Note: I am moving these tests in my axi branch to a different location. So maybe undo this change and I will deal with it on my side? Unless you really need it...
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 will undo this. I do not think I need it.
Description & Motivation
These are two components needed to build various kinds of cache memory.
First is a CAM which is a contents-addressable memory. It addresses memory by using a tag instead of an address.
It primarily returns a 'hit' but also the address of where it is found (we can work to make this optional).
The CAM also provides a standard write interface for populating the tags as data, using the line address.
Second is a Replacement module which is abstract and one kind of policy implemented; pseudoLRU. This will eventually be an optional Module passed into a Cache that can be used to modify a Cache with different way replacement policies.
Related Issue(s)
None.
Testing
Basic testing of the CAM to allow for simultaneous tag writes then tag lookups.
Backwards-compatibility
No.
Documentation
No.