Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] [VTA] [TSIM] Enabling Cycle-Accurate Hardware Simulation for VTA #3009 #3010

Merged
merged 35 commits into from
May 8, 2019
Merged

Conversation

vegaluisjose
Copy link
Member

RFC in #3009

@jroesch
Copy link
Member

jroesch commented Apr 12, 2019

Do we need to commit the Verilog code? isn't it generated?

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See additional reviews in the comment

cmake/modules/VTA.cmake Outdated Show resolved Hide resolved
vta/src/verilator/dpi_module.cc Outdated Show resolved Hide resolved
vta/include/vta/verilator/tsim.h Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Apr 16, 2019

High-level structural comments, given that dpi part(verilog shell and c file) of the codebase is compiled with the verilator. We should consider a distinct folder structure. Here is one possible recommendation. cc @jroesch @tmoreau89

/vta
/vta/dpi  
      - VTATSimMemHost.v
      - vta_tsim_dpi.h 
      - vta_tsim_dpi.cc
      - README.md
/vta/include/vta
     - dpi_module.h (the common dpi module defs that can be re-used)
/vta/src
     - dpi_module.cc (consider links to vta instead of tvm runtime, import vta will import the dpi)

@tqchen tqchen added the status: need update need update based on feedbacks label Apr 20, 2019
vta/apps/tsim/README.md Outdated Show resolved Hide resolved
vta/apps/tsim/README.md Outdated Show resolved Hide resolved
vta/apps/tsim/CMakeLists.txt Outdated Show resolved Hide resolved
vta/include/vta/dpi/tsim.h Outdated Show resolved Hide resolved
vta/src/dpi/tsim.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 2, 2019

@jroesch @tmoreau89 please help to review this PR

@vegaluisjose
Copy link
Member Author

vegaluisjose commented May 2, 2019

Ok I have addressed most of the issues, except for unsigned long long complains cpplint is giving on arguments for DPI functions. The reason why I used this type is because this is the same type used by Verilator when it compiles 64-bit hardware types to C.

EDIT: I fixed this by creating a type for dpi-arguments and lint-off that. This is easier to modify in the future.

vta/src/dpi/module.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 7, 2019

@tmoreau89
Copy link
Contributor

In order not to confuse adopters of TSIM, who would like to use TSIM to simulate any hardware instead of VTA itself, can we have a guide that is not part of the install.md file aimed at VTA developers?

Once VTA can be simulated by TSIM we can merge it back.

docs/vta/install.md Outdated Show resolved Hide resolved
@tmoreau89
Copy link
Contributor

I've approved the changes @vegaluisjose @tqchen. There seems to be an issue with the pr-merge CI; perhaps the branch should be re-based?

@tqchen tqchen removed the status: need update need update based on feedbacks label May 8, 2019
@tqchen
Copy link
Member

tqchen commented May 8, 2019

@tmoreau89 the ci is now green

@tmoreau89 tmoreau89 merged commit a6d04b8 into apache:master May 8, 2019
@liangfu
Copy link
Member

liangfu commented May 9, 2019

Thanks @vegaluisjose and all the reviewers ! This is a great start for us to develop, evaluate and integrate chisel3 based implement of VTA.

wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
…apache#3009 (apache#3010)

* merge files

* move verilator to the right place

* change name to tsim

* add default rule to be build and run

* add README for tsim

* Update README.md

* add some structural feedback

* change name of VTASim to VTADPISim

* more renaming

* update comment

* add license

* fix indentation

* add switch for vta-tsim

* add more licenses

* update readme

* address some of the new feedback

* add some feedback from cpplint

* add one more whitespace

* pass pointer so linter is happy

* pass pointer so linter is happy

* README moved to vta documentation

* create types for dpi functions, so they can be handle easily

* fix pointer style

* add feedback from docs

* parametrize width data and pointers

* fix comments

* fix comment

* add comment to class

* add missing parameters

* move README back to tsim example

* add feedback

* add more comments and remove un-necessary argument in finish

* update comments

* fix cpplint

* fix doc
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
…apache#3009 (apache#3010)

* merge files

* move verilator to the right place

* change name to tsim

* add default rule to be build and run

* add README for tsim

* Update README.md

* add some structural feedback

* change name of VTASim to VTADPISim

* more renaming

* update comment

* add license

* fix indentation

* add switch for vta-tsim

* add more licenses

* update readme

* address some of the new feedback

* add some feedback from cpplint

* add one more whitespace

* pass pointer so linter is happy

* pass pointer so linter is happy

* README moved to vta documentation

* create types for dpi functions, so they can be handle easily

* fix pointer style

* add feedback from docs

* parametrize width data and pointers

* fix comments

* fix comment

* add comment to class

* add missing parameters

* move README back to tsim example

* add feedback

* add more comments and remove un-necessary argument in finish

* update comments

* fix cpplint

* fix doc
@vegaluisjose vegaluisjose deleted the tsim branch May 15, 2019 02:24
@SharpTVRepairDubai
Copy link

Great discussion, learned a lot. Thanks,
@misc{poster:Ganjeloo:2018
We repair all brands of tv. Get your tv repaired from the comfort of home. 100% safe & reliable Sharp TV Repair Dubai Service by our trusted and top-rated technicians.

tqchen pushed a commit to tqchen/tvm that referenced this pull request Mar 29, 2020
…apache#3009 (apache#3010)

* merge files

* move verilator to the right place

* change name to tsim

* add default rule to be build and run

* add README for tsim

* Update README.md

* add some structural feedback

* change name of VTASim to VTADPISim

* more renaming

* update comment

* add license

* fix indentation

* add switch for vta-tsim

* add more licenses

* update readme

* address some of the new feedback

* add some feedback from cpplint

* add one more whitespace

* pass pointer so linter is happy

* pass pointer so linter is happy

* README moved to vta documentation

* create types for dpi functions, so they can be handle easily

* fix pointer style

* add feedback from docs

* parametrize width data and pointers

* fix comments

* fix comment

* add comment to class

* add missing parameters

* move README back to tsim example

* add feedback

* add more comments and remove un-necessary argument in finish

* update comments

* fix cpplint

* fix doc
@lyxcliang
Copy link

Hi, I download the example at https://github.com/apache/tvm-vta/tree/main/apps/tsim_example, and install Verilator(4.216 2021-12-05 version) on Ubuntu18.04. when i make it(run_verilog), i got the fllowing error, how should i do?
image

@vegaluisjose
Copy link
Member Author

I think the DPI interface changed (the verilog files here), and the example (chisel) has to be update it to take into account that.

@lyxcliang
Copy link

lyxcliang commented Jan 17, 2022

use https://github.com/apache/tvm-vta/tree/87ce9acfae550d1a487746e9d06c2e250076e54c/apps/tsim_example was OK. but when run "python3 tests/python/verilog_accel.py", it reported: libhw.so: undefined symbol : VTASimDPI
image

the generated code is:
image

why?

@vegaluisjose
Copy link
Member Author

The implementation of VTASimDPI does not seems to be loaded, perhaps a path issue in CMAKE files.

@lyxcliang
Copy link

lyxcliang commented Jan 18, 2022

the build command is: verilator build included the "VTASimDPI.v"
image

@vzyknc
Copy link

vzyknc commented Feb 24, 2022

the build command is: verilator build included the "VTASimDPI.v" image

I have the same problem. Do you have any solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants