feat: Add support for blackbox modules in BELs#599
feat: Add support for blackbox modules in BELs#599EverythingElseWasAlreadyTaken wants to merge 1 commit intoFPGA-Research:mainfrom
Conversation
Signed-off-by: Jonas K. <j0nask@proton.me>
There was a problem hiding this comment.
Pull request overview
This pull request modifies the getTopModule() method in the Yosys JSON parser to also accept modules with the "blackbox" attribute in addition to modules with the "top" attribute. The change aims to support blackbox modules in Basic Element of Logic (BEL) definitions, which are fundamental building blocks in the FPGA fabric.
Changes:
- Modified
getTopModule()logic to return modules with either "top" or "blackbox" attributes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| for name, module in self.modules.items(): | ||
| if "top" in module.attributes: | ||
| if "top" in module.attributes or "blackbox" in module.attributes: |
There was a problem hiding this comment.
The logic change introduces a semantic problem. The method is called getTopModule() but now also returns modules with the "blackbox" attribute. In Yosys, "top" and "blackbox" are distinct concepts - "top" marks the top-level module of a design hierarchy, while "blackbox" marks primitive modules without implementation details. If a design contains both a top module and blackbox modules (which is common), this change will return whichever is encountered first in the dictionary iteration, which may not be the actual top module. Consider either: (1) prioritizing "top" over "blackbox" by checking for "top" first, or (2) if blackbox-only designs are intended, creating a separate method or documenting this behavior change explicitly.
| """Find and return the top-level module in the design. | ||
|
|
||
| The top module is identified by having a "top" attribute. | ||
|
|
||
| Returns | ||
| ------- | ||
| tuple[str, YosysModule] | ||
| A tuple containing: | ||
| - The name of the top-level module (str) | ||
| - The YosysModule object for the top-level module | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If no top module is found in the design. | ||
| """ |
There was a problem hiding this comment.
The docstring is now inaccurate. It states "The top module is identified by having a 'top' attribute" but the implementation now also accepts modules with a "blackbox" attribute. The docstring should be updated to reflect this change in behavior, explaining when and why blackbox modules are returned instead of top modules.
| """ | ||
| for name, module in self.modules.items(): | ||
| if "top" in module.attributes: | ||
| if "top" in module.attributes or "blackbox" in module.attributes: |
There was a problem hiding this comment.
Test coverage is missing for the new behavior. The existing tests only verify modules with "top" attributes and modules with no attributes. A test should be added to verify the behavior when a module has a "blackbox" attribute, and ideally a test for when both "top" and "blackbox" modules exist to ensure the correct one is returned.
|
This is fine, but I just like to understand why you need the |
2 participants
No description provided.