[Docs] Added Image Description of Benchmark Suite #24
Conversation
WalkthroughAdds three auto-generated Verilog netlist modules (get_clocks, get_pins, get_ports), updates the SDC-generation script to always include object patterns and prepend create_clock prerequisites, and adds an auto_generated README plus a small docs image addition. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
auto_generated/netlist_files/get_pins_netlist.v (1)
1-35: Verify file completeness.The file appears to be missing a final newline at the end. While not a critical issue, it's a common best practice to end source files with a newline character.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/images/Synopsys-Design-Constraints.pngis excluded by!**/*.pngdocs/images/Syntax-Suite.pngis excluded by!**/*.pngdocs/images/Timing-Suite.pngis excluded by!**/*.png
📒 Files selected for processing (3)
auto_generated/netlist_files/get_clocks_netlist.v(1 hunks)auto_generated/netlist_files/get_pins_netlist.v(1 hunks)auto_generated/netlist_files/get_ports_netlist.v(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate_sdc.py (1)
314-352:generate_get_pins: you’re iterating unused options → duplicate outputs and wasted generationAfter the change, only
-regexp/-nocase/-quietare actually appended, but the loop still enumerates combinations over["-hierarchical", "-hsc", "-filter", ...](Line 321). That will generate many identical commands and file duplicates. Tightenoptional_options(and update the docstring) to match what’s emitted.def generate_get_pins(): ''' - Required: - Optional: -hierarchical, -hsc, -filter, -regexp, -nocase(Valid only with -regexp), -quiet, -of_objects, patterns - Note: -hierarchical cannot be used with -of_objects + Optional: -regexp, -nocase (legal only with -regexp), -quiet ''' commands = [] #List containing all possible combinations of options - optional_options = ["-hierarchical", "-hsc", "-filter", "-regexp", "-nocase", "-quiet", "-of_objects", "patterns"] + optional_options = ["-regexp", "-nocase", "-quiet"]
🧹 Nitpick comments (2)
scripts/generate_sdc.py (2)
236-273:generate_get_ports: consider dropping the create_clock prerequisite and/or align pattern to-regexp
get_portsgenerally doesn’t require clocks to exist; prefixingcreate_clockmay be harmless but can add noise and (if the referenced port doesn’t exist) introduce avoidable tool errors in suites that only want to validateget_ports. Also, if-regexpis present, ensurepatternis a regex (not a glob-ish token like*in).- #Add create_clock prerequisites - pieces = ("create_clock -period 10 -name clk [get_ports clk]\n" + pieces) + # If you truly need prerequisites here, consider referencing a port that always exists + # in the benchmark netlist, or omit prerequisites for get_ports entirely. + # pieces = ("create_clock -period 10 -name clk [get_ports clk]\n" + pieces)- pattern = generate_pattern("port") + pattern = generate_pattern("port") # consider generate_pattern("port_regex") when -regexp is set pieces.append(pattern)
274-313:generate_get_clocks: prereq clocks should be conditional on the chosen clock patternYou always create 4 clocks (clk1/clk2/clk_gen/clock) even if the generated
get_clockspattern targets something else; this can create unnecessary constraints and failures if some ports don’t exist in a given netlist. Consider emitting prerequisites only for the clocks implied by the pattern (or ensure the netlists always define these ports).
| "clock": ["clk*", "clk_gen", "clock", "clk1"], | ||
| "port": ["data*", "*in", "*out", "valid_in"], | ||
| "pin": ["u1/*", "*/Q", "u2/D", "*/clk"], | ||
| "cell": ["inst*", "reg_*", "*_buffer"], | ||
| "net": ["net*", "n[0-9]*", "*_data"] | ||
| } |
There was a problem hiding this comment.
Ensure patterns match the selected mode (glob vs -regexp)
PATTERNS now feeds both plain mode and -regexp mode. Several entries (e.g., clk*, *in, */Q) look like glob patterns; under -regexp they may be invalid or at least semantically different than intended depending on the SDC implementation. Consider maintaining separate pattern banks (glob vs regex) or translating when -regexp is present.
PATTERNS = {
- "clock": ["clk*", "clk_gen", "clock", "clk1"],
- "port": ["data*", "*in", "*out", "valid_in"],
- "pin": ["u1/*", "*/Q", "u2/D", "*/clk"],
+ # Consider splitting into "glob" vs "regex" banks if -regexp is used.
+ "clock": ["clk*", "clk_gen", "clock", "clk1"],
+ "port": ["data*", "*in", "*out", "valid_in"],
+ "pin": ["u1/*", "*/Q", "u2/D", "*/clk"],
"cell": ["inst*", "reg_*", "*_buffer"],
"net": ["net*", "n[0-9]*", "*_data"]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "clock": ["clk*", "clk_gen", "clock", "clk1"], | |
| "port": ["data*", "*in", "*out", "valid_in"], | |
| "pin": ["u1/*", "*/Q", "u2/D", "*/clk"], | |
| "cell": ["inst*", "reg_*", "*_buffer"], | |
| "net": ["net*", "n[0-9]*", "*_data"] | |
| } | |
| PATTERNS = { | |
| # Consider splitting into "glob" vs "regex" banks if -regexp is used. | |
| "clock": ["clk*", "clk_gen", "clock", "clk1"], | |
| "port": ["data*", "*in", "*out", "valid_in"], | |
| "pin": ["u1/*", "*/Q", "u2/D", "*/clk"], | |
| "cell": ["inst*", "reg_*", "*_buffer"], | |
| "net": ["net*", "n[0-9]*", "*_data"] | |
| } |
🤖 Prompt for AI Agents
In scripts/generate_sdc.py around lines 103 to 108, PATTERNS currently contains
glob-style entries that are being used regardless of whether the SDC engine is
invoked in plain (glob) or -regexp mode; this can produce incorrect or invalid
matches. Fix by branching on the selected mode: if mode == "regexp" either (a)
maintain a separate PATTERNS_REGEXP mapping with proper regular expressions for
each key, or (b) programmatically translate glob patterns to equivalent regex
(escaping as needed and converting * -> .* and ? -> .) before emitting them;
validate converted patterns (e.g., compile them) and use the appropriate bank
when generating the SDC output.
There was a problem hiding this comment.
Coderabbit is correct here @Minchan-Kwon . All of the examples I see here are Glob, not regex. For now, I think its ok to just support Glob. If you want to support Regex, you would need some slightly different examples.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
AlexandreSinger
left a comment
AlexandreSinger
left a comment
There was a problem hiding this comment.
Looks great @Minchan-Kwon
A couple of comments.
| "clock": ["clk*", "clk_gen", "clock", "clk1"], | ||
| "port": ["data*", "*in", "*out", "valid_in"], | ||
| "pin": ["u1/*", "*/Q", "u2/D", "*/clk"], | ||
| "cell": ["inst*", "reg_*", "*_buffer"], | ||
| "net": ["net*", "n[0-9]*", "*_data"] | ||
| } |
There was a problem hiding this comment.
Coderabbit is correct here @Minchan-Kwon . All of the examples I see here are Glob, not regex. For now, I think its ok to just support Glob. If you want to support Regex, you would need some slightly different examples.
There was a problem hiding this comment.
I like this picture a lot. Can you please add a reference to it in the syntax document that I wrote. You can add images to the README. I think this belongs at the top of that page as a summary.
There was a problem hiding this comment.
I like this image as well, but it may not fit with the Syntax docs. You could add your own README in the autogen suite for this if you would like.
There was a problem hiding this comment.
Same with this one. Normally, I like to always have the images in documentation somewhere so they are easier to access.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
auto_generated/README.md (2)
1-1: Capitalize and hyphenate the title.Line 1 should follow standard capitalization conventions for hyphenated compound adjectives.
-# Auto Generated Suite +# Auto-Generated Suite
9-9: Simplify the phrasing for conciseness.Consider removing the redundant "of" to streamline the text.
-All of the SDCs in this suite have been generated using generate_sdc.py included in the repository. +All the SDCs in this suite have been generated using generate_sdc.py included in the repository.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
auto_generated/images/Syntax-Suite.pngis excluded by!**/*.pngauto_generated/images/Timing-Suite.pngis excluded by!**/*.pngdocs/images/Synopsys-Design-Constraints.pngis excluded by!**/*.png
📒 Files selected for processing (2)
auto_generated/README.md(1 hunks)docs/syntax/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/syntax/README.md
🧰 Additional context used
🪛 LanguageTool
auto_generated/README.md
[grammar] ~1-~1: Use a hyphen to join words.
Context: # Auto Generated Suite This is a benchmark sui...
(QB_NEW_EN_HYPHEN)
[style] ~8-~8: Consider removing “of” to be more concise
Context: ...uto_generated/images/Syntax-Suite.png) All of the SDCs in this suite have been generated ...
(ALL_OF_THE)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
auto_generated/README.md (1)
7-7: Use descriptive alt text instead of the filename.The alt text should be descriptive of the image content, not the filename. This ensures better accessibility and clarity.
- +
🧹 Nitpick comments (2)
auto_generated/README.md (2)
1-1: Use a hyphen to join compound adjective."Auto Generated" should be hyphenated as "Auto-Generated" when used as a modifier before the noun.
-# Auto Generated Suite +# Auto-Generated Suite
9-9: Simplify "All of the" to "All the" for conciseness.-All of the SDCs in this suite have been generated using generate_sdc.py included in the repository. +All the SDCs in this suite have been generated using generate_sdc.py included in the repository.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
auto_generated/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
auto_generated/README.md
[grammar] ~1-~1: Use a hyphen to join words.
Context: # Auto Generated Suite This is a benchmark sui...
(QB_NEW_EN_HYPHEN)
[style] ~8-~8: Consider removing “of” to be more concise
Context: ...-Suite.png](./images/Syntax-Suite.png) All of the SDCs in this suite have been generated ...
(ALL_OF_THE)
🔇 Additional comments (1)
auto_generated/README.md (1)
5-5: Path correction acknowledged.The relative path to the syntax documentation has been properly corrected from
./docs/syntax/README.mdto../docs/syntax/README.md.
2 participants
Added the images describing the benchmark suite
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.