Fix linter warnings detected by iverilog and add resize(foo,n) support#38
Fix linter warnings detected by iverilog and add resize(foo,n) support#38ApexGP wants to merge 4 commits into
iverilog and add resize(foo,n) support#38Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates vhd2vl’s parsing/emission to (a) eliminate iverilog -Wall warnings in generated example outputs and (b) add translation support for resize(vec, n) into explicit Verilog extension/truncation.
Changes:
- Add
resize(foo, n)handling in the parser, emitting explicit zero-extend or truncation logic in Verilog. - Enhance
conv_integer(...)handling in port maps by looking up formal port widths and emitting explicit 32-bit extension to avoidiverilogpadding warnings. - Update example VHDL sources and committed translated Verilog examples, plus bump GHDL example compilation to
--std=08.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/vhd2vl.y |
Adds entity port tracking for formal-width lookup, introduces param_list, and implements resize(...) + port-map extension logic. |
src/vhd2vl.l |
Ensures CONVFUNC_2 tokens carry the function name (needed to detect resize). |
examples/resize.vhd |
New VHDL example covering resize extension/truncation behavior. |
translated_examples/resize.v |
New expected translated Verilog for the resize example. |
examples/conv_integer.vhd |
Expands test coverage for conv_integer in port maps across const/signal/port and widths. |
translated_examples/conv_integer.v |
Updates expected translated Verilog to explicitly extend to 32-bit ports (avoids iverilog padding warnings). |
examples/agg_risk_demo.vhd |
Adjusts example to avoid an always @* block with no sensitivities (adds real inputs). |
translated_examples/agg_risk_demo.v |
Updates expected Verilog accordingly so always @* is sensitivity-driven. |
examples/Makefile |
Uses VHDL-2008 mode (--std=08) when compiling examples with GHDL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sl = addsl(NULL, expr_to_sl(vec)); | ||
| sl = addtxt(sl, "["); | ||
| sl = addval(sl, width_val - 1); | ||
| sl = addtxt(sl, ":0]"); |
There was a problem hiding this comment.
build_resize() truncation emits vec[width_val-1:0], which assumes the source vector is 0-based. This will produce incorrect/out-of-range selects for signals declared with non-zero low bounds (the translator already emits such ranges, e.g. [15:8]). Consider carrying the source vrange (or at least its nlo) into build_resize and slicing [nlo+width_val-1:nlo] (or an equivalent part-select) instead of hardcoding :0.
| sl = addsl(NULL, expr_to_sl(vec)); | |
| sl = addtxt(sl, "["); | |
| sl = addval(sl, width_val - 1); | |
| sl = addtxt(sl, ":0]"); | |
| /* Without knowing the original low index of 'vec', emitting | |
| * [width_val-1:0] would only be correct for 0-based vectors | |
| * and can result in out-of-range or incorrect selects when | |
| * translating signals declared with non-zero low bounds. | |
| * To avoid generating invalid Verilog, fall back to the | |
| * unmodified expression here instead of slicing. | |
| */ | |
| sl = addsl(NULL, expr_to_sl(vec)); |
| if (e->op == 'S') { | ||
| sl = addsl(sl, inner); | ||
| sl = addtxt(sl, "["); | ||
| sl = addval(sl, src - 1); | ||
| sl = addtxt(sl, "]}},"); |
There was a problem hiding this comment.
Signed extension in build_conv_portmap_ext() uses inner[src-1] as the sign bit, which assumes the vector’s MSB index equals (width-1). For vectors with non-zero low bounds (e.g. [15:8]) this points at the wrong bit and will sign-extend incorrectly. Use the declared MSB index from the symbol’s vrange (e.g. range->nhi) rather than (src-1).
| sg = lookup(io_list, e->sl->data.txt); | ||
| if (!sg) { | ||
| sg = lookup(sig_list, e->sl->data.txt); | ||
| } |
There was a problem hiding this comment.
get_src_width() only looks up identifiers in io_list and sig_list, but resize(vec,w) should also work when vec is a VHDL constant (which is emitted as a Verilog parameter and is now tracked in param_list). Add a param_list lookup here so resize() can determine widths for constants/parameters too.
| } | |
| } | |
| if (!sg) { | |
| sg = lookup(param_list, e->sl->data.txt); | |
| } |
| map_item : mvalue {$$=$1;} | ||
| | NAME '=' '>' mvalue { | ||
| | NAME '=' '>' { portmap_ctx.formal=$1; } mvalue { | ||
| slist *sl; |
There was a problem hiding this comment.
portmap_ctx.formal is set for named associations but never cleared for positional map_item alternatives. This means a later positional port map (or a later positional item after a named one) can reuse a stale formal name and apply width-extension against the wrong port. Clear portmap_ctx.formal (and possibly compnt) when entering a new map_item that is positional, or immediately after finishing each map_item.
- Use `vrange->nlo` for truncation in `buiild_resize()` when known - Add `param_list` lookup in `get_src_width()` - Fix sign exts for vecs with non-zero low bounds
|
Hi @ldoolitt,
Looking forward to your thoughts! |
|
Small patch const1.txt to make your work const-correct. |
|
Hi @ldoolitt, The core issue was managing the Here is a breakdown of the specific actions:
According to the Bison documentation, these midrule actions are executed as soon as the preceding symbols are shifted, which is exactly what we need to ensure the context is ready for the nested expressions. LMK if this approach aligns with your vision for the parser's state management. |
|
Hi @ldoolitt, just following up on this PR. All CI and linter checks are green. I've also verified the |
|
You can find the latest patch here. |
Hi @ldoolitt, I realized the previous scope of this PR might have been too broad. To make the review process more manageable, I’ve moved the I've also performed a final cleanup of the remaining commits to improve code quality and maintainability:
Looking forward to your feedback. |
This PR is meant to address two issues
agg_risk_demo.vhdandconv_integer.vhd: See Lint detected by iverilog in example output #37resize(foo,n): See Testcases added #31 (comment). Support forconv_integer(foo)has been added in AddCONVFUNC_1support #35 .