-
Notifications
You must be signed in to change notification settings - Fork 298
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
[ExportChiselInterface] Support probe types #5497
Conversation
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.
Just curious, what's this for these days / what motivates this change?
Please don't change FIRRTLType's in a pass PR, see review comment. It looks like this is also unnecessary so all the more reason.
I'm not sure it matters (what's the story for this pass? are you using it? Is it experimental, only for exploring ideas? Should it be actually maintained/robust? Should it be under test, what version(s) of Chisel does it require/expect...?)-- but if you're adding this presumably it's used somehow so can't help but wonder how this works for either Foreign types or (WIP) Properties, right?
Anyway, looks like this pass would hit the unreachable if encountering those constructs, which is bad. FWIW/FYI-- unreachable is not for expressing you don't support a behavior, it literally informs the compiler that code can be assumed unreachable and optimize accordingly, leading to bugs/crashes should that not be true.
As an alternative, could this just emit a diagnostic about the unsupported type instead?
@@ -82,6 +82,10 @@ class FIRRTLType : public Type { | |||
// Convenience methods for accessing recursive type properties | |||
//===--------------------------------------------------------------------===// | |||
|
|||
/// Return true if this is a "passive" type - one that contains no "flip" | |||
/// types recursively within itself. | |||
bool isPassive() const { return getRecursiveTypeProperties().isPassive; } |
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 sort of change does not belong on a PR to ExportChiselInterface, please split out any such changes to their own PR.
I don't think this is right, there's a reason it's on FIRRTLBaseType. This is only reasonable to ask of things that can be in hardware and this should be kept at the same level as getPassiveType()
which also belongs on FIRRTLBaseType
for a few reasons.
(but this should all be reworked, anyway, but not as a modification to the export chisel interface pass; especially since it doesn't seem this is something actually used on a FIRRTLType but for convenience where you know/expect/require it's otherwise a FIRRTLBaseType anyway?)
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'll open a new pr with just this change, but I figured it was perfectly valid based on this comment that ref types are explicitly not passive
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.
It could be a method on RefType
but if you already know you have a reftype you probably don't need to ask the question (and if that's not clear your code is probably doing the wrong thing).
The point is it doesn't generalize to non-hw types-- for example, Property types-- even if the current RTP approach requires all types put something in that bit, and while there's an answer for RefType's it's also rather misleading re:implication of flow of data through it and what (rw)probe's are.
Anyway if you'd like to help untangle this that'd be great, it's something I hope to push on soon-ish (tm).
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 see, I'll just remove it 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.
Removed and reworked to pass in a hardware type if present, which avoids unnecessary casting. I'm holding off on explicitly supporting property types until they are more baked/there is a Chisel representation of them.
Direction direction, llvm::raw_ostream &os, | ||
unsigned int indent, | ||
bool &containsProbeType, unsigned int indent, |
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.
Out parameter in the middle is a little rough, maybe rework? I see the default-valued argument prevents the normal convention of putting it last, but there are many ways this could be structured (perhaps with state + iterative?).
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, the parameters were getting unwieldy. Updated to use a class to store state and the stream.
bool emitDirection = | ||
type.isPassive() && !type.containsAnalog() && !hasEmittedDirection; | ||
type.isa<RefType>() || | ||
(type.isPassive() && !type.containsAnalog() && !hasEmittedDirection); |
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.
Hyper-nit: Should !hasEmittedDirection
go first as it's local and trivial to check (and when going through recursive types will often be false?).
I haven't thought much about it, but what's the reasoning here? Why only emit direction for passive things?
Analog kinda is weird re:being legal to be input
vs output
at all. Is output analog not reachable from Chisel?
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 about hasEmittedDirection
.
Chisel direction functions override any internal directions. So doing Output(new Bundle {...})
erases all direction information inside the bundle. That's why directions are only placed on the outermost passive members. And they otherwise don't matter for Analog
.
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'll add a comment explaining that
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.
Awesome, thanks for the explanation re:Chisel. TIL! 👍
This is definitely experimental, but is a piece of a build flow for putting a component on "the shelf" that doesn't need to conform to a Chisel interface ahead of time (i.e. you can conform the generated extmodule to an interface later on). This might go away but it's not much work to maintain in the meantime. The usage of this pass is tested, I can point it to you offline.
Yeah, that makes more sense. |
c9fe64e
to
ad63110
Compare
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.
Thank you, this looks great!
Didn't super carefully audit the refactor but from what you say and bounce through this looks good.
Thank you for the rework, and appreciate the comments and structure, and error test! 🚀
No description provided.