Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[FIRRTL] Add ExtModule Verification #162
Conversation
seldridge
force-pushed the
seldridge:firrtl-extmodule-verification
branch
from
4cb2253
to
ba13215
|
Looks nice assuming this is the correct semantics for the conflict check, thanks! |
Add a method, getWidthlessType, to FIRRTLType that strips the widths from that FIRRTLType. This is used in situations where you are trying to do some structural equivalence of names and types, but don't want to consider widths. Signed-off-by: Schuyler Eldridge <[email protected]>
Adds verification of the FIRRTL Circuit operand to reject circuits with invalid FExtModules. This checks the following things: 1. No FExtModule defname conflicts with an FModule symbol name 2. All FExtModules with the same defnames have the same number of ports 3. All FExtModules with the same defnames have ports with the same name in the same order. 4. All FExtModules with the same defname have exactly the same type of port including widths if they take no parameters and exactly the same type of port excluding widths if they take parameters. Adds tests of the above behavior. This aligns FIRRTL Dialect behavior to accept and reject the same circuits that the Scala FIRRTL Compiler accepts and rejects in its test suits that involve ExtModules. Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed the
seldridge:firrtl-extmodule-verification
branch
from
9120de8
to
2941584
seldridge
changed the title
|
cool, thank you for working on this! |
|
Nice, just a couple of suggestions to further simplify the code, thank you for implementing this! |
| // Go to next extmodule if no extmodule with the same defname | ||
| // was found. | ||
| if (!collidingExtModule) | ||
| continue; |
lattner
Oct 23, 2020
Collaborator
would it make sense to pull this into the 'if' above? Something like: if (!value) { value = extModule; continue }. This eliminates the "else" by making it be the fall through.
Just my personal style, but I wouldn't worry about adding the extra scope around the "value" reference. Once you do that, you can move the definition of collidingExtModule lower.
would it make sense to pull this into the 'if' above? Something like: if (!value) { value = extModule; continue }. This eliminates the "else" by making it be the fall through.
Just my personal style, but I wouldn't worry about adding the extra scope around the "value" reference. Once you do that, you can move the definition of collidingExtModule lower.
seldridge
Oct 23, 2020
Author
Member
Yeah, it's cleaner to just move the continue up. I can also remove the scope guarding of value by just putting that in the if statement.
Yeah, it's cleaner to just move the continue up. I can also remove the scope guarding of value by just putting that in the if statement.
| for (auto p : llvm::zip(ports, collidingPorts)) { | ||
| StringAttr aName, bName; | ||
| FIRRTLType aType, bType; | ||
| std::forward_as_tuple(std::tie(aName, aType), std::tie(bName, bType)) = p; |
lattner
Oct 23, 2020
Collaborator
Whoa, I've never seen forward_as_tuple before :-). I tend to use lower abstraction code for clarity, but this is fine.
Whoa, I've never seen forward_as_tuple before :-). I tend to use lower abstraction code for clarity, but this is fine.
|
nice work! |
Adds checking of FIRRTL extmodule (
FExtModuleOp) that brings the FIRRTL Dialect verification in line with the examples that I extracted from the Scala FIRRTL Compiler tests. A FIRRTL circuit is now verified such that the following properties hold:To help with this, this PR adds a method,
getWidthlessTypetoFIRRTLTypethat sets all the widths to unknown to facilitate a check like this.Towards #80.