-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[VTA][TSIM][Build] Towards TSIM CI testing #3704
Conversation
@vegaluisjose @liangfu please review, thanks! |
Hey @tmoreau89 thanks for leading this. Isn't going back to enable through CMAKE a similar thing than having it in the Regarding the simulation libraries ( Also, if we decided to go this route we could even have a way of generating a library for your target say |
Thanks for the feedback Luis, I think we can build TSIM by default in either simulation modes then in order to always generate have both When in |
@vegaluisjose I've made the change so that in either simulation modes, we always build both |
@tmoreau89 This is a great idea with careful engineering work! I would agree with @vegaluisjose to rename the libraries into option(build_vta_sim "Build VTA with fast simulation support." ON)
option(build_vta_tsim "Build VTA with cycle-accurate simulation support." ON) , and For the benefit of doing this, here I quote from @vegaluisjose 's comment,
|
@liangfu this is an interesting suggestion; essentially control the build of runtime DLLs from the CMAKE, and control what hardware design gets emulated from the One suggestion would be to have a |
Actually, this may be a little more complicated due to the simple reason that depending on the target (say |
Therefore, if we want to have CMAKE controlled build for the simulator sources, we may need yet another parameter for the VTA RPC when we build on device (e.g. pynq). We'd essentially have 3 options:
|
This is fine by me.
I switched the testing between
I personally think this is a better design. |
OK sounds like we are on the same page! Please see the latest commit - this will let us explicitly set what dlls to build (either TSIM, FSIM, or FPGA driver); then the json target determines what FPGA is targeted. Right now it's only used when building the FPGA driver, but we could also use it to set the TSIM top-level design, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note before merging, I'll need to update the docs to reflect the build steps for simulation or to run on FPGA. So please do not merge right away! |
@liangfu quick update: one suggestion that was made was to have a single parameter that indicates whether we are building on host or on FPGA: if it's on host, we build both simulator runtimes, and if it's on FPGA, we build the pynq runtime (or other if it's another vendor). How does that sound? |
I suggest we hold from merging this PR until #3721 is merged, and we can enable TSIM unit testing. |
@tmoreau89 I would personally prefer the explicit way for this, in order to configure what to build separately. |
Thanks for the feedback Liangfu. |
We'll stick with the 3 flags in the config.cmake |
#3721 is merged, and I've enabled TSIM CI testing! |
* building TSIM specific library along with fast simulator to quickly switch between dlls * cmake controlled TSIM libraries * always build tsim driver in either simulation modes * build DLLs based on CMAKE flags * updating the jenkinsfile * small restructuring * reducing the cmake flags * update instructions * reverting to 3 flags * update Jenkinsfile * adding new line * enabling TSIM unit and integration tests * fix description * temporarily disabling task_python_vta tests in CPU Build stage * move CPU tests in unit test stage * stage reorg * better make * disabling TSIM tests for now * reverting some restructuring * fix
* building TSIM specific library along with fast simulator to quickly switch between dlls * cmake controlled TSIM libraries * always build tsim driver in either simulation modes * build DLLs based on CMAKE flags * updating the jenkinsfile * small restructuring * reducing the cmake flags * update instructions * reverting to 3 flags * update Jenkinsfile * adding new line * enabling TSIM unit and integration tests * fix description * temporarily disabling task_python_vta tests in CPU Build stage * move CPU tests in unit test stage * stage reorg * better make * disabling TSIM tests for now * reverting some restructuring * fix
* building TSIM specific library along with fast simulator to quickly switch between dlls * cmake controlled TSIM libraries * always build tsim driver in either simulation modes * build DLLs based on CMAKE flags * updating the jenkinsfile * small restructuring * reducing the cmake flags * update instructions * reverting to 3 flags * update Jenkinsfile * adding new line * enabling TSIM unit and integration tests * fix description * temporarily disabling task_python_vta tests in CPU Build stage * move CPU tests in unit test stage * stage reorg * better make * disabling TSIM tests for now * reverting some restructuring * fix
Can you explain where you declare in |
@apivovarov I think |
@liangfu Can you reproduce the error? Try to build with LLVM
Install TVM from Source https://docs.tvm.ai/install/from_source.html |
* building TSIM specific library along with fast simulator to quickly switch between dlls * cmake controlled TSIM libraries * always build tsim driver in either simulation modes * build DLLs based on CMAKE flags * updating the jenkinsfile * small restructuring * reducing the cmake flags * update instructions * reverting to 3 flags * update Jenkinsfile * adding new line * enabling TSIM unit and integration tests * fix description * temporarily disabling task_python_vta tests in CPU Build stage * move CPU tests in unit test stage * stage reorg * better make * disabling TSIM tests for now * reverting some restructuring * fix
Before both build and execution would be controlled by the
TARGET
field in thevta_config.json
. Now we separate the build (set in cmake with theUSE_TSIM
field) from the execution (set by theTARGET
field in thevta_config.json
.This change presents the following advantages:
Update: now that #3721 is merged, I'm also enabling TSIM CI testing so we can make sure that TSIM support does not break moving forward. I've updated the title to reflect this major feature update.