-
Notifications
You must be signed in to change notification settings - Fork 613
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
[util] Add a withShadowLayer Queue #4589
Conversation
I think with my suggested tweak this would become much more useful... |
The Probe.apply API is supposed to take a second argument of type `Layer`. However, this was incorrectly taking an argument of type `Option[Layer]`. Revert to the former. Signed-off-by: Schuyler Eldridge <[email protected]>
Add a method to the `Queue$` object that can be used to create a `Queue` and include a second queue that contains some data which will be tracked alongside the first `Queue`, but this will be kept in a layer. This is added to help address a use case where design verification wants to track some extra, sidecar data that should be added to some data pipeline. However, this sidecar data is expected to be removed from the design. This would normally be addressed with a generator-time parameter. However, the generator-time parameter prevents removal of the sidecar data unless the design is re-elaborated. Signed-off-by: Schuyler Eldridge <[email protected]>
c5ff54a
to
a56480f
Compare
val shadowQueue = Queue(shadowEnq, entries, pipe, flow, useSyncReadMem, flush.map(BoringUtils.tapAndRead)) | ||
|
||
val _shadowDeq = Wire(Valid(chiselTypeOf(data))) | ||
_shadowDeq.valid :<= shadowQueue.valid |
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.
Should we assert that this valid always equals the original valid...? Is there a reason they should not?
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 should always be the same. If they're not then it's a bug in this generator.
Maybe we want something that is a layer for the purposes of testing the generator itself. 🧐 I'm going to take no action on this for now.
* @return output (dequeue) interface from the queue and a [[ShadowFactory]] | ||
* for creating shadow [[Queue]]s | ||
*/ | ||
def withShadow[T <: Data]( |
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.
Shouldn't this have two data types? Shadow is probably not the same type as T...?
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.
Interestingly, no, due to how ShadowFactory
works. A ShadowFactory
takes a type parameter that is the same as the queue it was created from. However, it has a method that has a different type parameter that is the type of the shadow.
I.e., if you start with a Queue[UInt]
, you get a ShadowFactory[UInt]
. However, that has a method apply[A <: Data]
where A
is the type of the new shadow queue. This A
isn't fixed, however. I.e., if you create a ShadowFactory
, you then want to create an A
that can be a UInt
, SInt
, or any user-defined Bundle
.
If withShadow
took a second type parameter, then it would be locked in to creating shadow queues of only that type (or it would be something overly pessimistic like Data
requiring asTypeOf
casts). (I originally ran into this problem and switched this to the ShadowFactory
approach to avoid it!)
Sound good?
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.
sure, I agree with what you've said that we don't need to pass the desired shadow type through, though it still seems like the ShadowFactory type might as well just be Data
... I am not sure how the fact that the original queue was of type T
is used at all by the code in ShadowFactory? not blocking
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.
oh! i see you have changed this so that ShadowFactory
no longer takes the T parameter 👍
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.
Yeah, your intuition was right here in that it doesn't need the type parameter at all. That was a nice suggestion.
95a3370 includes a |
82c46c2 adds clock/reset capturing to make sure that when the shadow queue is created it is using the same clock and reset that the original queue is. There is no way to override this, however, it would be very unsafe to do so. |
Add a method to the
Queue$
object that can be used to create aQueue
and include a second queue that contains some data which will be tracked alongside the firstQueue
, but this will be kept in a layer.This is added to help address a use case where design verification wants to track some extra, sidecar data that should be added to some data pipeline. However, this sidecar data is expected to be removed from the design. This would normally be addressed with a generator-time parameter. However, the generator-time parameter prevents removal of the sidecar data unless the design is re-elaborated.
E.g., below, this can be used to create a shadow queue outside the module which contains a queue. The shadow queue will operate in lockstep with the original queue:
This compiles to the following Verilog:
Release Notes
Queue.withShadow
utility that assists with adding data to a queue that will be relegated to a specific layer.