ghdl-yosys-plugin icon indicating copy to clipboard operation
ghdl-yosys-plugin copied to clipboard

[WIP] Implement a VHDL backend

Open rlee287 opened this issue 5 years ago • 9 comments

This will be a VHDL backend from #112, and I will be using this PR to track what has been finished and what still needs work.

  • [ ] Adjust command line options as necessary
  • [ ] Ensure identifiers are valid VHDL identifiers
    • [x] Keyword collisions (merely updating the keyword list?)
    • [ ] Handling $
    • [x] Extended identifiers (now used when syntactically needed and to avoid collisions with internally generated names)
  • [x] Adjust Makefiles
  • [x] Get GHDL version to put into generated output ~(see ghdl/ghdl#1448)~
  • [ ] Technical debt (as outlined in comment below)
  • [ ] Write tests for the backend
  • [ ] Figure out how to dump attributes cleanly
  • [x] Better handling of sensitivity lists (helper function to extract these from SigSpecs?)
  • [ ] Dump memory cells correctly (not just type declarations in dump_memory)
  • [ ] Use qualified expressions when aggregates have ambiguous array type
  • [ ] Other high-level issues that may appear

On extended identifiers: #112 suggests always using these, but user-provided names (e.g. port names) should be preserved when possible, and IEEE-1076-2008 15.4.3 states that "Moreover, every extended identifier is distinct from any basic identifier."

Functions that need to be changed to output VHDL syntax (note that this checklist only deals with syntax generation in that function and not any other things that may need adjustment):

  • [x] void reset_auto_counter(RTLIL::Module *module)
  • [x] std::string next_auto_id()
  • [x] std::string id(RTLIL::IdString internal_id, bool may_rename = true)
  • [x] bool is_reg_wire(RTLIL::SigSpec sig, std::string &reg_name)
  • [x] void dump_const(std::ostream &f, const RTLIL::Const &data, int width = -1, int offset = 0, bool no_decimal = false, bool escape_comment = false)
  • [x] void dump_reg_init(std::ostream &f, SigSpec sig)
  • [x] void dump_sigchunk(std::ostream &f, const RTLIL::SigChunk &chunk, bool no_decimal = false)
  • [x] void dump_sigspec(std::ostream &f, const RTLIL::SigSpec &sig)
  • [ ] void dump_attributes(std::ostream &f, std::string indent, dict<RTLIL::IdString, RTLIL::Const> &attributes, char term = '\n', bool modattr = false, bool regattr = false, bool as_comment = false)
  • [x] void dump_wire(std::ostream &f, std::string indent, RTLIL::Wire *wire)
  • [x] void dump_memory_types(std::ostream &f, std::string indent, Mem &mem)
  • [ ] void dump_memory(????, std::string indent, Mem &mem) (split off on my own, signature to be determined when implemented
  • [x] void dump_cell_expr_port(std::ostream &f, RTLIL::Cell *cell, std::string port, bool gen_signed = true)
  • [x] std::string cellname(RTLIL::Cell *cell)
  • [x] void dump_cell_expr_uniop(std::ostream &f, std::string indent, RTLIL::Cell *cell, std::string op)
  • [x] void dump_cell_expr_binop(std::ostream &f, std::string indent, RTLIL::Cell *cell, std::string op)
  • [ ] bool dump_cell_expr(std::ostream &f, std::string indent, RTLIL::Cell *cell)
  • [ ] void dump_cell(std::ostream &f, std::string indent, RTLIL::Cell *cell)
  • [x] void dump_conn(std::ostream &f, std::string indent, const RTLIL::SigSpec &left, const RTLIL::SigSpec &right)
  • [ ] void dump_proc_switch(std::ostream &f, std::string indent, RTLIL::SwitchRule *sw)
  • [ ] void dump_case_body(std::ostream &f, std::string indent, RTLIL::CaseRule *cs, bool omit_trailing_begin = false)
  • [ ] void dump_process(std::ostream &f, std::string indent, RTLIL::Process *proc, bool find_regs = false)
  • [x] void dump_module(std::ostream &f, std::string indent, RTLIL::Module *module)
  • [x] void VHDLBackend::execute(std::ostream *&f, std::string filename, std::vector<std::string> args, RTLIL::Design *design) YS_OVERRIDE

rlee287 avatar May 22 '20 00:05 rlee287

Documentation of major design decisions I am making (feel free to contest these in followup comments):

  • All signals and variables will be std_logic_vectors, with casts to unsigned and signed as necessary for arithmetic operations. (This mirrors the current behavior of ghdl --synth.)
  • As all signals will be a vector type with fixed width, -decimal (which does not include widths in constants) does not make sense and will be removed.
  • It may be possible to represent RTLIL::Process objects exactly in VHDL, so that warning can be removed once this is confirmed.
  • $specify* type cells ($specify2, $specify3, and $specrule cells to be more specific) are not translated. A warning is printed, and a comment is left behind in the generated code indicating that the cell was not translated.
  • As per a conversation in the Gitter chat, Verilog (and RTLIL by extension) does not differentiate between arrays of 1 element and singular values (i.e. STD_LOGIC_VECTOR (0 downto 0) and STD_LOGIC), so the backend will dump using STD_LOGIC. A side effect of this is that vector ports of width 1 will turn into non-vector ports upon export, causing type mismatches.
  • Include a --std08 option that uses some elements of the VHDL-08 in order to make the generated code more human-readable.

rlee287 avatar May 23 '20 02:05 rlee287

Technical debt to cleanup before finishing:

  • [ ] Everything labelled TODO in the code
  • [x] Array types are generated with the name array_type_(width), so the signal renaming logic should be changed to check for collisions with this
  • [ ] Check if there is a reason for stringf("string_constant") and remove the wrapping function call if there isn't one
  • [ ] Check if is_reg_wire and related are actually required for VHDL (since the reg/wire distinction does not exist in VHDL), and possibly using is_reg_wire for more informative signal names
  • [ ] Signal concatenation on the LHS of an expression most likely assumes VHDL-2008 and probably generates invalid output for multi-chunk SigSpecs when -std08 is not specified

rlee287 avatar May 24 '20 01:05 rlee287

FYI, VlogHammer might come in useful for testing this. The patch below is an attempt at adapting it for ghdl (to be run with make YOSYS_MODE=ghdl GHDL_MODULE=/path/to/ghdl.so syn_yosys), but there's not enough functionality to properly test it yet:

diff --git a/scripts/syn_yosys.sh b/scripts/syn_yosys.sh
index cb2244f..5276c8d 100644
--- a/scripts/syn_yosys.sh
+++ b/scripts/syn_yosys.sh
@@ -55,6 +55,9 @@ case "$YOSYS_MODE" in
        7|minimal)
                yosys -q -l synth.log -b 'verilog -noattr' -o synth.v \
                      -p 'hierarchy; proc' ../../rtl/$job.v ;;
+       8|ghdl)
+               yosys -m "${GHDL_MODULE:-ghdl}" -q -l synth.log -b 'verilog -noattr' -o synth.v \
+                     -p 'write_vhdl -noattr synth.vhd; design -reset; ghdl synth.vhd -e' ../../rtl/$job.v ;;
        *)
                echo "Unsupported mode: $YOSYS_MODE" >&2
                exit 1 ;;

Xiretza avatar May 30 '20 11:05 Xiretza

It turns out that more of the id related functions need updating, as the autogenerated ids need to be changed to be valid identifiers (without underscores at the beginning or end). I will need to decide if I want to imitate ghdl --synth and also append _o or _q to internal signal names. (The checklist at the top will be updated momentarily.)

rlee287 avatar Jun 04 '20 06:06 rlee287

Finally getting around to resuming development and copying over the Mem refactoring, but the requirement for placing signal declarations at the beginning for VHDL means that the actual memory dumping is going to be more complicated.

rlee287 avatar Dec 17 '20 06:12 rlee287

Not quite. "=" is a string operation. "?=" is a hardware operation. "=" like character matches like character "?=" 0 matches L, 1 matches H, '-' matches anything. U results in U (except with -), everything else is X.
For more see the LRM.

JimLewis avatar Dec 17 '20 21:12 JimLewis

Had a long discussion with a Verilog person about what value to assign in the default assignment within a case statement. It actually helped me understand why I should keep doing what I had always been doing. Which is when assigning to a signal, use 'X" as don't care. For example:

   case slv2 is 
     when "00" => ...
     when "01" => ...
     when "10" => ...
     when "11" => ...
     when others => Y <= "XXXX" ;
  end case ; 

If instead, Y were assigned to "----", then the expression, Y ?= "0000" would be true when the don't care happened. OTOH, interesting things would happen anyway if the condition "11" were not covered and it actually happened in the real hardware.

JimLewis avatar Dec 17 '20 21:12 JimLewis

Hi! Great initiative @rlee287! I saw there was no activity on this PR so I went a little further with it:

https://github.com/kammoh/ghdl-yosys-plugin/tree/vhdl_backend

I'm now able to run post-synthesis simulation using Yosys synth/synth_xilinx netlist synthesis and GHDL simulation. I've only tested a couple of rather small designs and it passes verification. I merged in master and made sure works with the latest yosys.

Please let me know your thoughts about those changes.

Would you still be interested in working on this feature @rlee287? I'd love to provide any help, as this is a feature I direly need at the moment. Otherwise please let me know if I should make a new PR.

Thank you!

kammoh avatar Feb 02 '22 02:02 kammoh

Unfortunately, I haven't had time to continue working on this, so I appreciate you stepping up to continue where I left off. I hope you'll be able to see this through and merely ask that you continue documenting design choices and similar things (e.g. the last commit of Yosys that this part of the plugin was based on, in order to track whether it accounts for updates to any of the Yosys cell libs, etc.), similar to how I documented such things when I was working on this earlier.

If you do complete this and get a PR of yours merged with this functionality, I'll be happy to close this PR. Thanks again for picking up where I left off.

rlee287 avatar Feb 04 '22 23:02 rlee287