synth: match slang \$-suffixed module names in SYNTH_KEEP_MODULES#4175
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the synthesis script to ensure parameterized modules are preserved during hierarchy optimization by expanding the selection pattern to include $-suffixed canonical names. A technical issue was identified where the new selection pattern might fail to match RTLIL canonical names because Yosys does not automatically prepend backslashes when wildcards are present; a more robust selection command using the -module flag and a leading wildcard was suggested.
The slang HDL frontend produces `\$`-suffixed canonical RTLIL names for parameterized module instantiations, so `select -module <name>` misses them and `keep_hierarchy` silently doesn't get applied. Replace with a two-pattern `select` that matches both the bare name and `<name>\$*`. The second pattern matches nothing when no `\$`-suffix names exist, so the builtin and verific frontends are unchanged. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
oharboe
force-pushed
the
orfs-synth-keep-slang-select
branch
from
8718306 to
3355586
Compare
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the module selection logic in synth.tcl to support parameterized instances generated by the slang frontend. However, the current implementation of the select command incorrectly handles multiple patterns on the Yosys stack, which can lead to the base module being ignored or an empty selection. It is recommended to use the union operator %u and retain the -module flag to ensure correct behavior across different frontends.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Gemini flagged that `select A B` might not union the two patterns, which would regress the non-slang case where only the bare name exists. It does union them -- yosys/passes/cmds/select.cc:1494 runs `while (work_stack.size() > 1) select_op_union(...)` at the end of select -- but make the intent obvious by explicitly stacking `%u`, and expand the comment so future readers don't have to trace select.cc to convince themselves the non-slang case still works. No behavior change; verified with yosys 0.63 against a design carrying both a renamed `\bar$2` (slang-style canonical) and a plain `\top`: both receive `keep_hierarchy` under the two-pattern select whether or not `%u` is present. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
The previous commit added a cosmetic `%u` to quiet a review concern
about `select A B` not unioning its patterns. It does union them, and
the `%u` is redundant. Drop the `%u` and instead document the two
yosys behaviors that make the construct correct, citing the source
file and function names so future readers don't have to rediscover
them:
* match_ids retries the pattern against the id with a leading `\`
stripped, so the wildcard pattern matches canonical names like
`\foo$1` without needing a `\`-prefix in the Tcl string.
* The select command auto-unions remaining patterns on the work
stack when it finishes parsing, so a pattern that matches nothing
only produces a warning -- the other pattern still applies.
Also note why we don't use `-module <name>`: it errors out when the
module doesn't exist, which is exactly the case this change is trying
to handle.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@maliberty I don't know what to do about the security check: |
|
We look for the word |
Should I do something? |
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
2 participants
The slang HDL frontend produces
\$-suffixed canonical RTLIL names for parameterized module instantiations, soselect -module <name>misses them andkeep_hierarchysilently doesn't get applied.Replace with a two-pattern
selectthat matches both the bare name and<name>\$*. The second pattern matches nothing when no\$-suffix names exist, so the builtin and verific frontends are unchanged.