OFS / opae-sdk

Open Programmable Acceleration Engine
https://ofs.github.io
BSD 3-Clause "New" or "Revised" License
258 stars 82 forks source link

ASE fails to simulate IP cores with sub-libraries that contain non-verilog/system-verilog files #1142

Closed dwilson1394 closed 5 years ago

dwilson1394 commented 5 years ago

1. Missing VHDL files during Modelsim compilation When trying to simulate a design containing a floating IP core for inverse square root (that I name a10_flt_inv_sqrt_300MHz), make sim will have an error such as

Instantiation of 'a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i' failed. The design unit was not found.

My IP core generates the following directory structure,

a10_flt_inv_sqrt_300MHz
|-- altera_fp_functions_170
|    |-- synth
|         |-- a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i.vhd
|         |-- a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i_memoryC0_uid58_invSqrtTables_lutmem.hex
|         |-- a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i_memoryC1_uid61_invSqrtTables_lutmem.hex
|         |-- a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i_memoryC2_uid64_invSqrtTables_lutmem.hex
|         |-- dspba_library.vhd
|         |-- dspba_library_package.vhd
|-- synth
    |-- a10_flt_inv_sqrt_300MHz.v

As a workaround, I can successfully run make sim by adding the vhdl files in the correct compile order to the generated vhdl_files.list (if there were prior vhdl files in the project).

2. Missing hex files during Modelsim simulation After running a simulation on the same design with a hard-coded input value of 2 (0x40000000), the corresponding output is shown incorrectly as 0x00000000.

As a workaround, I can get the correct output by copying the hex files to ASE's work directory.

Point of interest generate_ase_environment.py adds the generated verilog/system verilog files to a list that is appended to the vlog_files.list. Since vhdl files can be generated in the sub-libraries, despite the generation target being verilog, the script may need to also handle vhdl files, in addition to hex files. https://github.com/OPAE/opae-sdk/blob/c7cc92445db5e08ab6a077c8cfe2a0171fa3778e/ase/scripts/generate_ase_environment.py#L468-L472

lzhan55 commented 5 years ago

@dwilson1394 Did you use afu_sim_setup to set up the simulation for your design containing a floating IP core for inverse square root? How to reproduce your issue?

There is a pull request at https://github.com/OPAE/opae-sdk/pull/698? Could that help with your issue? I will merge that changes into trunk soon.

dwilson1394 commented 5 years ago

I used afu_sim_setup and had the following setup:

Setup:

Steps to Reproduce

  1. Setup a design containing the floating IP core for inverse square root.
    1. For my test, I modified https://github.com/OPAE/intel-fpga-bbb/tree/master/samples/tutorial/01_hello_world
    2. Copied the qsys ip core to the hw/rtl folder
    3. Added the qsys ip to sources.txt
    4. Added an instantiation in cci_hello_afu.sv with a hard-coded input of 0x40000000 and a dummy signal to the output
  2. Setup and run ASE
    afu_sim_setup --sources hw/rtl/sources.txt test
    cd test
    make
    make sim

The pull request at https://github.com/OPAE/opae-sdk/pull/698 actually creates more errors. Those changes add the vhd files to the list of qsys generated files (i.e. qsys_sim_files.list) which is appended to vlog_files.list and compiled by Modelsim's vlog execution. Since vlog is only intended for verilog files, vlog encounters errors for the vhdl syntax:

Error: (vlog-13069) /root/intel-fpga-bbb/samples/tutorial/01_hello_world/test2/qsys_sim/root/intel-fpga-bbb/samples/tutorial/01_hello_world/hw/rtl/a10_flt_inv_sqrt_300MHz/altera_fp_functions_171/synth/dspba_library_package.vhd(1): near "--": syntax error, unexpected --, expecting class.

You would need a separate file to collect the generated vhd qsys files (in correct compile order) that is appended to vhdl_files.list, which is compiled by vcom.

In addition, this change does nothing for the hex files.

lzhan55 commented 5 years ago

@michael-adler Could you help with this issue?

lzhan55 commented 5 years ago

@dwilson1394 Regarding the copying of hex file of "As a workaround, I can get the correct output by copying the hex files to ASE's work directory", where did you copy the hex file from?

dwilson1394 commented 5 years ago

From the IP core's generated files (where test is the folder generated by afu_sim_setup) e.g. test/qsys_sim/../a10_flt_inv_sqrt_300MHz/altera_fp_functions_170/synth/a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i_memoryC0_uid58_invSqrtTables_lutmem.hex

If I'm understanding the hex file issue, one of the generated source files (a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i.vhd) has a relative path to the hex file. When it's compiled by modelsim, the compiled entity uses that path but relative to modelsim's work directory (rather than where the generated files are). I'm guessing either the hex files need to be in the work directory or the source file's relative path needs to be updated relative to modelsim's work directory.

michael-adler commented 5 years ago

It would save me time (and I'd get to this sooner) if you can provide a copy of the configuration you are trying so simulate. The updated sources.txt, Qsys files, RTL, etc. If you don't have a place to store it and post a URL here we can come up with another method of transferring the code.

Thank you.

dwilson1394 commented 5 years ago

project.tar.gz This is https://github.com/OPAE/intel-fpga-bbb/tree/master/samples/tutorial/01_hello_world with a dummy instantiation of a floating-point inverse square root ip core

The IP core has a hard-coded value of 2 as input and has a dummy output signal test_output.

michael-adler commented 5 years ago

https://github.com/OPAE/opae-sdk/pull/1148

dwilson1394 commented 5 years ago

That branch handles the issue on my setup.

Another potential issue would be how the vhdl files are ordered in qsys_vhdl_sim_files.list. In this case, a10_flt_inv_sqrt_300MHz_altera_fp_functions_171_hcdip5i.vhd and dspba_library.vhd are dependent on dspba_library_package.vhd and must be placed behind it.

Although I cannot induce a wrong ordering in my setup, you can manually modify the ordering in qsys_vhdl_sim_files.list and see that Modelsim complains when dspba_library.vhd is placed before dspba_library_package.vhd.

In this implementation, the order is based on os.walk, which uses os.listdir(?) and has an arbitrary order (https://docs.python.org/2/library/os.html#os.listdir). Potentially, this means certain users could get the wrong ordering... maybe?

michael-adler commented 5 years ago

Good catch. I will think about it tomorrow.

michael-adler commented 5 years ago

I added a sort to the same branch with https://github.com/OPAE/opae-sdk/pull/1148. I wish it weren't just a heuristic, but we are stuck using --synth mode in Qsys due to the way some IP is configured and it doesn't emit any metadata to help construct a build order.

Thank you for noting the dependence on order.

dwilson1394 commented 5 years ago

The changes still work for my setup.

If you're interested in a potential alternative, you could reorder the qsys_vhdl_sim_files.list in the Makefile after compiling the quartus simulation libraries through brute forcing.

In this approach, you take multiple compiling passes at uncompiled files in the list, removing successfully compiled files from the candidates and stopping either when all files have been compiled or if no files had successfully compiled in a single pass. The "correct" order is then the order at which they successfully compiled during the passes which would replace the order of qsys_vhdl_sim_files.list.

This also might be a nice option to have for a user's vhdl files, assuming there's no strange consequences of using this approach.

michael-adler commented 5 years ago

I'd prefer to just get it right.

FYI, I just put in another tweak as https://github.com/OPAE/opae-sdk/pull/1152, undoing a nasty side effect of the Makefile change that was forcing pointless rebuilding of the megafunction simulation libraries.

Thank you for your report.