Project-Zipline icon indicating copy to clipboard operation
Project-Zipline copied to clipboard

Vivado Implementation Issues

Open ANILATHOMAS519 opened this issue 5 years ago • 31 comments

Hi @michaelgmcintyre , While trying to implement this project in Vivado, we found some syntax errors and some ports which were wrongly declared, like in file:

  • [ ] https://github.com/opencomputeproject/Project-Zipline/blob/master/rtl/cr_prefix/cr_prefix_fe_ctlr.v

Can you please clarify whether these are bugs or any mistakes that we are overlooking from our side?

ANILATHOMAS519 avatar Jun 21 '19 10:06 ANILATHOMAS519

Hi ANILATHOMAS519,

Please provide specific details on both syntax errors and wrongly declared ports. Project was developed using synopsys tool flow. See zipline.setup for specific version.

Thanks, Michael

michaelgmcintyre avatar Jun 21 '19 15:06 michaelgmcintyre

  1. Project-Zipline/rtl/cr_prefix_attach/cr_prefix_fe _ctlr.v

    • Line 32 : "usr_ib_rd","fe_ctlr_cmd_tlv","fe_ctlr_cmd_tlv_valid"=> These ports are declared in Module Declaration, but has not been mentioned whether they are input or output.

    • Line 36 : "usr_ib_empty", "usr_ib_aempty", "usr_ib_tlv" => Have these three variables used in this file?

    • Line 53-55 : " input logic [63:0] ibc_char_in; } input logic [63:0] ibc_char_in_valid; }
      input logic [7:0] ibc_char_vbytes; } output fe_ready; } output fe_eodb; } => Are these five variables input ,output or just logic ? (since they have not mentioned inside module declaration as ports).

    • Line 281 : Is there an "end" missing in DATA_BLK corresponding to the "begin"?

  2. Project-Zipline/rtl/cr_prefix_attach/cr_prefix_attach_ctlr.v

    • Line 107 : assign pfd_mem_addr = {pac_prefix_num,pac_pfcounter};
      => Is it "pac_pfcounter" or "pac_pfdcounter"?(Because in Line 97 "pac_pfdcounter" is declared.)

    • Line 108 : assign phd_mem_addr = {pac_prefix_num,pac_hfcounter};
      => Is it "pac_hfcounter" or "pac_phdcounter"?(Because in Line 98 "pac_phdcounter" is declared.)

    • Line 110 : assign pac_phdcounter_max = (pac_phdcounter == N_PHD_WORDS); => Is it "N_PHD_WORDS" or "CR_PREFIX_N_PHD_WORDS"? (Since N_PHD_WORDS are not declared anywhere. In the file "Project-Zipline/rtl/common/include/cr_global_params.vh" CR_PREFIX_N_PHD_WORDS has been declared in line 118)

    • Line 111 : assign pac_pfdcounter_max = (pac_pfdcounter == N_PFD_WORDS); => Is it "N_PFD_WORDS" or "CR_PREFIX_N_PFD_WORDS"? (Since N_PFD_WORDS are not declared anywhere.In the file "Project-Zipline/rtl/common/include/cr_global_params.vh" CR_PREFIX_N_PFD_WORDS has been declared in line 119.)

  3. Project-Zipline/rtl/common/cr_mux_32.v

  *Line 14        :       wire   0  = t0 | t1;
                                                                  =>Is it "0" or "o"?

ANILATHOMAS519 avatar Jun 24 '19 07:06 ANILATHOMAS519

Hi @michaelgmcintyre ,

   As you mentioned above , I have given the details of syntax errors and their respective location in the files . Can you please check it whether these are errors or not?   We are using Vivado 18.3 tool for this project.

Syntax errors noticed and it's file location:

  1. Project-Zipline/rtl/cr_prefix_attach/cr_prefix_fe _ctlr.v

    • Line 32 : "usr_ib_rd","fe_ctlr_cmd_tlv","fe_ctlr_cmd_tlv_valid"=> These ports are declared in Module Declaration,but has not been mentioned whether they are input or output.

    • Line 36 : "usr_ib_empty", "usr_ib_aempty", "usr_ib_tlv" => Have these three variables used in this file?

    • Line 53-55 : " input logic [63:0] ibc_char_in; } input logic [63:0] ibc_char_in_valid; }
      input logic [7:0] ibc_char_vbytes; } output fe_ready; } output fe_eodb; } => Are these five variables input ,output or just logic ? (since they have not mentioned inside module declaration as ports).

    • Line 281 : Is there an "end" missing in DATA_BLK corresponding to the "begin"?

  2. Project-Zipline/rtl/cr_prefix_attach/cr_prefix_attach_ctlr.v

    • Line 107 : assign pfd_mem_addr = {pac_prefix_num,pac_pfcounter};
      => Is it "pac_pfcounter" or "pac_pfdcounter"?(Because in Line 97 "pac_pfdcounter" is declared.)

    • Line 108 : assign phd_mem_addr = {pac_prefix_num,pac_hfcounter};
      => Is it "pac_hfcounter" or "pac_phdcounter"?(Because in Line 98 "pac_phdcounter" is declared.)

    • Line 110 : assign pac_phdcounter_max = (pac_phdcounter == N_PHD_WORDS); => Is it "N_PHD_WORDS" or "CR_PREFIX_N_PHD_WORDS" ? (Since N_PHD_WORDS are not declared anywhere. In the file "Project-Zipline/rtl/common/include/cr_global_params.vh" CR_PREFIX_N_PHD_WORDS has been declared in line 118)

    • Line 111 : assign pac_pfdcounter_max = (pac_pfdcounter == N_PFD_WORDS); => Is it "N_PFD_WORDS" or "CR_PREFIX_N_PFD_WORDS"? (Since N_PFD_WORDS are not declared anywhere.In the file "Project-Zipline/rtl/common/include/cr_global_params.vh" CR_PREFIX_N_PFD_WORDS has been declared in line 119.)

  3. Project-Zipline/rtl/common/cr_mux_32.v

  *Line 14        :       wire   0  = t0 | t1;
                                                                  =>Is it "0" or "o"?

ANILATHOMAS519 avatar Jun 24 '19 08:06 ANILATHOMAS519

Thanks ANILATHOMAS519.

Please provide the error and warning messages coming from Vivado when you attempt to elaborate.

michaelgmcintyre avatar Jun 24 '19 23:06 michaelgmcintyre

Thanks for your response.

                                                                          This is the screenshot of the error and critical warning shown when we compile only CCEIP_64 part of the project.
                                                                          

Screenshot from 2019-06-25 09-40-34

 We tried after removing the syntax errors, after that we were able to do away with the critical warnings. But the errors remain the same. The screenshot given below shows the synthesis error what we got after removing syntax errors.

11Screenshot from 2019-06-25 10-09-33

ANILATHOMAS519 avatar Jun 25 '19 04:06 ANILATHOMAS519

Hi ANILATHOMAS519,

I looked through the modules that generated the errors and failures. Nothing stands out specifically to me. May well be implementation of Verilog parsing syntax between Synopsis and Xilinx toolchains.

For example, the error "conditional expression could not be resolved to a constant [cr_huf_comp_lut.v:335]"

  for (genvar i=0; i<N_SYMBOLS_ENTRY; i++)
  . . .

Most likely can be resolved by rearranging the code to:

  genvar i;
  for (i=0; i<N_SYMBOLS_ENTRY; i++)
  . . .

You may need to modify files for your toolchain.

Thanks, Michael

michaelgmcintyre avatar Jun 25 '19 16:06 michaelgmcintyre

Thanks for your sincere support . As per your suggestion, we are doing on that error in project . I have given declaration of "i" outside for loop , but the synthesis error didn't gone . We are looking into the issue related to that "for" loop and will revert back to you(since for loop is not seem to be synthesizable in vivado).

Thank you Anila

ANILATHOMAS519 avatar Jun 27 '19 12:06 ANILATHOMAS519

I'm a little surprised the declaration of "i" outside the for loop didn't resolve the issue. That Verilog block is pretty straight forward. The only other thing I can see there would be how the "generate" statement works in vivado. You may try to move the "genvar" statement outside the "generate" block:

genvar i;
generate
    for (i=0; i<N_SYMBOLS_ENTRY; i++)
        begin : hw_mem
             . . . 
        end
endgenerate	

Please consult the vivado doumentation concerning variable declaration, especially for the use of the "generate" block.

It would be useful for this project to add a file describing what needs to be changed or how to get the vivado toolchain to work if you are able to resolve the issues.

Thanks, Michael

michaelgmcintyre avatar Jun 27 '19 16:06 michaelgmcintyre

We are looking on that error as per your suggestion. We failed again when genvar is given outside of the loop. We will try to provide the document if we succeed . Thanks Anila

ANILATHOMAS519 avatar Jun 28 '19 11:06 ANILATHOMAS519

Hi @michaelgmcintyre ,

We tried implementing "for" loops in an independent project and that was synthesized successfully.So only error which we can doubt to create the problem is as given below. Can you please take a look at the below mentioned error :

[Synth 8-3391] Unable to infer a block/distributed RAM for 'g.mem_reg' because the memory pattern used is not supported. Failed to dissolve the memory into bits because the number of bits (73728) is too large. Use 'set_param synth.elaboration.rodinMoreOptions {rt::set_parameter dissolveMemorySizeLimit 73728}' to allow the memory to be dissolved into individual bits

Our doubt is that this above mentioned error can be the reason for the generation of the remaining errors which can be seen in the screenshot which was posted in this thread itself.

Can this error be related to over utilization of resources ?

We tried setting the parameter given in the error but again it throw the same error with this time the number of bits being even bigger(155360).

Regards Anila

ANILATHOMAS519 avatar Jul 01 '19 13:07 ANILATHOMAS519

Hi Anila,

Please make sure you understand the Verilog code based on the keywords. It is good that you can synthesis "for" loops, but in the example above you need to use the "generate" block for the code to produce the correct results.

As to the block/distributed RAM issue, I don't think I can help since this issue does not occur with the Synopsis toolchain. However, I did find the links below for Vivado which may help.

https://forums.xilinx.com/t5/Vivado-TCL-Community/Why-is-the-Vivado-HD-unable-to-infer-a-block-distributed-RAM/td-p/304489

Also, please review the Vivado documentation on the application of distributed RAM and block RAM. You may need to contact your Xilinx FAE for additional support.

Thanks, Michael

michaelgmcintyre avatar Jul 01 '19 23:07 michaelgmcintyre

Hi @michaelgmcintyre ,

   We read the link which you have given. The solution which we can see is that   "set parameter" which we have implemented. But the error remained same. We are continuing to take a look based on Vivado documentation of the "application of distributed RAM and block RAM". 

On the log file of synthesis, the warning corresponding to the error is

** WARNING: [Synth 8-4767] Trying to implement RAM 'g.mem_reg' in registers. Block RAM or DRAM implementation is not possible; see log for reasons. Reason is one or more of the following : 1: RAM has multiple writes via different ports in same process. If RAM inferencing intended, write to one port per process. 2: Unable to determine number of words or word size in RAM. 3: No valid read/write found for RAM.

Since warning is not mentioning the correct location of the warning, we are having a hard time locating which module/file would be responsible for this. Can you please suggest whether any of the mentioned cases can be a problem?

Thanks for your support Anila

ANILATHOMAS519 avatar Jul 02 '19 12:07 ANILATHOMAS519

Anila,

How does the error change when you apply the following as suggested in the elaboration error:

set_param synth.elaboration.rodinMoreOptions {rt::set_parameter dissolveMemorySizeLimit 73728}

Thanks, Michael

michaelgmcintyre avatar Jul 02 '19 16:07 michaelgmcintyre

Hi @michaelgmcintyre , After applying : "set_param synth.elaboration.rodinMoreOptions {rt::set_parameter dissolveMemorySizeLimit 73728}", we encountered the same error with the "Sizelimit" being 1563000 and the error corresponding to "for"(error of for loop:"conditional expression could not be resolved to a constant") loop was solved.

Then we again gave "set_param synth.elaboration.rodinMoreOptions {rt::set_parameter dissolveMemorySizeLimit 1563000}"

Attached the screenshot of the synthesis error at this point for your reference scrrnsht of log file of cr_xp10_decomp and cr_cceip_64 error(latest error)

Now the error files have changed from "huf_comp files" to "xp10 files". We are now looking into these errors and will let you know the results.

ANILATHOMAS519 avatar Jul 03 '19 11:07 ANILATHOMAS519

@ANILATHOMAS519 Did you ever manage to solve your problems? I'm trying to simulate the zipline_tb.v and had no success with vivado simulator or questa advances. I ran into the same syntax problems you've decribed (I suspect some lines were accidently deleted when part of this project has been made open source). If you managed to get is working with the vivado toolchain, sharing your fork of the repo would be much appreceated.

Meradanis avatar Aug 26 '19 13:08 Meradanis

Hi Meradanis,

We pulled the project after posting to ensure it could be elaborated and simulation ran. It was successful on our toolchain. I'll try to pull again to ensure this.

Has anyone using vivado toohchain attempted to contact Xilinx or a FAE for assistaince with the issues you are encountering?

Thanks, Michael

michaelgmcintyre avatar Aug 26 '19 15:08 michaelgmcintyre

@michaelgmcintyre I made a new github account, I used my private one (Meradanis) earlier.

Right now, I'm not even using the vivado toolchain. I postponed that step until I managed to simulate. I would like to to the CCE64 simulation example. I modified the makefile to use Questa Advance instead of Synopsis. I am having trouble compiling the example. One of the errors:

** Error: /zipline/Project-Zipline/dv/CCE_64/run/../../../rtl/cr_xp10_decomp/cr_xp10_decomp_fe_bhp_dflate.v(182): (vlog-2163) Macro 'CR_DFLATE_ID1 is undefined.

That macro is defined in cr_xp10_decomp.vh which is not included in cr_xp10_decomp_fe_bhp_dflate.v nor in any of the packages imported there.

When I add the `include manually, it works until it finds the next file with missing macros. I wonder if synopsis vlogan somehow imports the verilog headers globally. If so, I did not find that command in the makefile.

Any advice would be much appreceated.

ghost avatar Aug 28 '19 09:08 ghost

I managed to get the simulation with Questa Advanced up and running. For anyone wondering: You have to compile all source files a single unit (option -mfcu) and then it works.

The CDE and KME simulations all pass their tests, the CCE only passes for some of the xp10 cases, zlib gzip and the rest all fails. I will update when I know more.

ghost avatar Aug 30 '19 13:08 ghost

Thanks for posting, bastianboese.

Can you post additional information that you are seeing for failed CCE test cases for xp10, zlib, and gzip?

michaelgmcintyre avatar Sep 03 '19 16:09 michaelgmcintyre

@michaelgmcintyre I uploaded log files of the failed CCE test. 1.zlib_sv.log 1.gzip_sv.log 1.smoke_sv.log

As comparison, I here is a logfile of a successfull CCE test: 1.xp9_sv.log

Apart from changing the Makefiles to run with Questa Advanced, I had to change line 14 of rtl/common/cr_mux_32.v. I renamed that wire from "0" to "o0", since our toolchain would not simulate a wire named "0".

ghost avatar Oct 18 '19 12:10 ghost

Would it be possible to release an EDIF netlist of the project? then all syntax issues would go away at the user side. This EDIF file can be synthesized in the original tool-chain, then the users can instantiate the EDIF as a black box in Vivado or Quartus or anything. Possibly you already have it on your computer.

Does this code include any hard macros like block rams or PLLs, that are device specific? That would make it extremely hard to re-use this code.

Does the Synopsis tool-chain used for developing the original code have settings that list what they call the language used? For example "system verilog 2018" or something?

buenoshun avatar Oct 28 '19 18:10 buenoshun

Hi buenoshun,

Thanks for your suggestions. Let me investigate.

Thanks, Michael

michaelgmcintyre avatar Oct 29 '19 22:10 michaelgmcintyre

Hi buenoshun,

Q: Would it be possible to release an EDIF netlist of the project? A: Unfortunately, we are unable to publicly release the netlist.

Q: Does this code include any hard macros like block rams or PLLs, that are device specific? A: No. All device specific memories to register-based behavioral memories were replaced, so no hard macro block rams/PLLs.

Q: Does the Synopsis tool-chain used for developing the original code have settings that list what they call the language used? A: The Open Sourced RTL provided requires Verilog-2001 and Verilog extentiuon in Accellera SystemVerilog specification enabled for compilation.

Thanks, Michael

michaelgmcintyre avatar Oct 30 '19 16:10 michaelgmcintyre

Thanks. So, it is "SystemVerilog 3.1a", an extension to the standard verilog "IEEE 1364-2001". http://www.ece.uah.edu/~gaede/cpe526/SystemVerilog_3.1a.pdf " extensive set of high-level abstraction extensions, as definedin this document"

xilinx ug900 https://forums.xilinx.com/xlnx/attachments/xlnx/SIMANDVERIBD/21055/1/Appendix_G.pdf The Vivado® Integrated Design Environment (IDE) supports the following languages: •VHDL, see IEEE Standard VHDL Language Reference Manual (IEEE-STD-1076-1993) [Ref 15] •Verilog, see IEEE Standard Verilog Hardware Description Language(IEEE-STD-1364-2001) [Ref 16] •SystemVerilog Synthesizable subset. See IEEE Standard Verilog Hardware Description Language (IEEE-STD-1800-2009) [Ref 17] •IEEE P1735 encryption, see Recommended Practice for Encryption and Management of Electronic Design Intellectual Property (IP) (IEEE-STD-P1735) see Table G-8:Verilog Language Support Exceptions ?? Shouldn't IEEE-STD-1800-2009 include SV3.1a ?? If yes, then Vivado should not have problems with it, but it does. According to IEEE "SystemVerilog 3.1a refers to the Accellera SystemVerilog 3.1a Language Reference Manual [B3],a precursor to IEEE Std 1800-2005".

Having distributed ram in the code is good and bad in the same time. Good that we don't relay on elements that don't exist in our device, but it may be bad if the amount required is larger than we have even in the largest FPGA devices. Someone complained above about the memory size, although there is not enough context there, I don't know what size FPGA device he/she used. If it does not fit, then I dont know what the solution is. Maybe the users have to rewrite the code with xilinx block ram.

What is the purpose of this open source release? Educational, or design-reuse? It seems that design re-use for FPGAs would only work in this case with the EDIF netlist. Or we can only re-use it for ASIC, but that is cost prohibitive for most users. If the RAM is an issue, then EDIF would not help either.

buenoshun avatar Oct 30 '19 19:10 buenoshun

Hi Buenoshun

The purpose of the Open Source release is both for educational purposes and as a baseline for design reuse.

From the educational perspective, the release of all the collateral, design specifications, microarchitecture specifications, and RTL, as a contribution to the Open Source community provides our insight and approach to solving a growing issue with cloud computing - how to maximize storage via a lossless compression algorithm/implementation of data at real-time line rates.

From the design reuse perspective, it provides all the RTL we developed during the multi-year Corsica ASIC program (https://azure.microsoft.com/en-us/blog/improved-cloud-service-performance-through-asic-acceleration/). This represents a significant amount of research, architectureal design, implementation, and verfication. We've Open Sourced all of it! We built it using a specific ASIC tool flow (Synopsis) with the hope that others will be able to find utility in it.

Thanks, Michael

michaelgmcintyre avatar Nov 06 '19 22:11 michaelgmcintyre

@ANILATHOMAS519 Did you ever get it to work on xilinx FPGA with vivado?

evangle avatar Feb 24 '20 21:02 evangle

@michaelgmcintyre Hi, we are facing the same distribute memory issue when trying to implement it with Xilinx FPGA and toolchain. You mention you guy design it using synopsis ASIC toolchain (I guess is DC). Since there is an FPGA parameter switch in the code, did you guys also synthesize it in FPGA, and what toolchain do you use, Synplify? Thanks

evangle avatar Aug 26 '20 15:08 evangle

@michaelgmcintyre Hi, I am getting error that vivado is not able to implement the complex assignment. Example like this " v_bit_buf[v_s1_buf_idx +: 4]" also when I am trying to find the hard coded value of the variable "v_s1_buf_idx" it gets track to the variable "r_s1_buf_idx" whose reference I am not able to find ahead. I am not able to track down the actual declaration of this variable.

If anyone could help me solving this problem then it would be great.

Thanks, Deepen

deenee13 avatar Sep 16 '20 20:09 deenee13

@michaelgmcintyre Hi, we are facing the same distribute memory issue when trying to implement it with Xilinx FPGA and toolchain. You mention you guy design it using synopsis ASIC toolchain (I guess is DC). Since there is an FPGA parameter switch in the code, did you guys also synthesize it in FPGA, and what toolchain do you use, Synplify? Thanks

I found out something interesting. Module nx_ram_2rw defined a 2 write ports mem which cannot be mapped to BRAM. However, each instance of nx_ram_2rw only uses port A for reading and port B for writing. So, we can comment out the line of mem[_adda] <= dina_i; to overcome this issue.

Wolf-Tungsten avatar Jan 16 '21 11:01 Wolf-Tungsten

@michaelgmcintyre Hi, I am getting error that vivado is not able to implement the complex assignment. Example like this " v_bit_buf[v_s1_buf_idx +: 4]" also when I am trying to find the hard coded value of the variable "v_s1_buf_idx" it gets track to the variable "r_s1_buf_idx" whose reference I am not able to find ahead. I am not able to track down the actual declaration of this variable.

If anyone could help me solving this problem then it would be great.

Thanks, Deepen

Hi, @deenee13, I try to use vivado to synthesis the project, and get the same error with you, did you solve it?

newcoder1234567 avatar Feb 07 '22 08:02 newcoder1234567