opencomputeproject / Project-Zipline

Defines a lossless compressed data format that is independent of CPU type, operating system, file system, and character set, and is suitable for compression using the XP10 algorithm.
Other
277 stars 47 forks source link

Vivado Implementation Issues #8

Open ANILATHOMAS519 opened 5 years ago

ANILATHOMAS519 commented 5 years ago

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:

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

michaelgmcintyre commented 5 years ago

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

ANILATHOMAS519 commented 5 years ago

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 commented 5 years ago

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"?
michaelgmcintyre commented 5 years ago

Thanks ANILATHOMAS519.

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

ANILATHOMAS519 commented 5 years ago

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

michaelgmcintyre commented 5 years ago

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

ANILATHOMAS519 commented 5 years ago

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

michaelgmcintyre commented 5 years ago

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

ANILATHOMAS519 commented 5 years ago

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 commented 5 years ago

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

michaelgmcintyre commented 5 years ago

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

ANILATHOMAS519 commented 5 years ago

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

michaelgmcintyre commented 5 years ago

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

ANILATHOMAS519 commented 5 years ago

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.

Meradanis commented 4 years ago

@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.

michaelgmcintyre commented 4 years ago

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

ghost commented 4 years ago

@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 commented 4 years ago

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.

michaelgmcintyre commented 4 years ago

Thanks for posting, bastianboese.

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

ghost commented 4 years ago

@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".

buenoshun commented 4 years ago

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?

michaelgmcintyre commented 4 years ago

Hi buenoshun,

Thanks for your suggestions. Let me investigate.

Thanks, Michael

michaelgmcintyre commented 4 years ago

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

buenoshun commented 4 years ago

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.

michaelgmcintyre commented 4 years ago

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

evangle commented 4 years ago

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

evangle commented 3 years ago

@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

deenee13 commented 3 years ago

@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

Wolf-Tungsten commented 3 years ago

@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.

newcoder1234567 commented 2 years ago

@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?

Wolf-Tungsten commented 2 years ago

I tried to put together a version that can be synthesized via Vivado. But in the end, the resources of the FPGA (Alveo U280) could not meet the requirements. https://github.com/Wolf-Tungsten/Project-Zipline-Alveo-U280