OPEN: Snitch Cluster Tiling Support #22
Conversation
extend snitch support
Victor-Jung
changed the title
tahaelbayad
changed the title
.github/workflows/BuildDocker.yml
Outdated
| tags: ghcr.io/pulp-platform/deeploy:main | ||
| tags: ghcr.io/tahaelbayad/deeploy:main |
There was a problem hiding this comment.
Why changing the tag if you don't modify the docker build process?
There was a problem hiding this comment.
You're right. I did that before the previous PR was merged but forgot to update it afterwards. I updated it here 45cd6ec.
.github/workflows/CI.yml
Outdated
| image: ghcr.io/pulp-platform/deeploy:main | ||
| image: ghcr.io/tahaelbayad/deeploy:main |
There was a problem hiding this comment.
Same comment as before, no need to change the tag if you don't change anything in the Docker
.github/workflows/CI.yml
Outdated
| testRQGEMMTransB | ||
|
|
||
| snitch-kernels-tiled-singlebuffer-L2: | ||
| uses: ./.github/workflows/TestRunnerTiledSnitch.yml |
There was a problem hiding this comment.
This TestRunner should be renamed TestRunnerTiledSnitchSequential.
In the CI flow you can spawn parallel jobs for each test or execute many tests sequentially in the same job. Which one to use (parallel or sequential) really depends on how long the test takes. Each time you spawn a job, a VM starts on the GitHub servers, then it loads the Deeploy docker, and finally starts the test. Loading the docker takes ~50sec and each time you spawn a new job you have to pay this price. Now, if your test takes 5sec (which is the case for small networks) it's better to spawn one job and run all the test sequentially.
There was a problem hiding this comment.
I changed the name to TestRunnerTiledSnitchSequential and adapted it so that number of cores and default memory level are input variables here 0d4baed. Plus, I made the same adjustment for the --simulator flag, let me know if you find this useful.
| L1_values=$(echo "$test" | jq -r '.L1[]') | ||
| for L1_value in $L1_values; do | ||
| echo "Running test: $testName with L1: $L1_value" | ||
| python testRunner_tiled_snitch.py -t Tests/$testName --cores=9 --simulator=banshee --l1 $L1_value --defaultMemLevel=L2 --toolchain_install_dir /app/install/riscv-llvm/ |
There was a problem hiding this comment.
Move the number of cores to an input variable of this reusable workflow, just like it's done in TestRunnerTiledSiracusaSequential, give it a default value of 9 too. Maybe in the future we want to have some test running on 1 core.
There was a problem hiding this comment.
Same for the default memory level
There was a problem hiding this comment.
As mentioned before you can check it here 0d4baed
.gitmodules
Outdated
| path = TargetLibraries/Snitch/third_party/pulp-nn-mixed | ||
| url = https://github.com/Victor-Jung/pulp-nn-mixed.git | ||
| branch = deeploySnitchTarget |
There was a problem hiding this comment.
I don't think this is necessary anymore. We just have one kernel from this library (requantized add), so let's move this kernel in the Snitch library (TargetLibraries/Snitch/kernel) instead of adding a complete submodule just for this.
There was a problem hiding this comment.
You can check the changes here 6fe7c42
This PR mirrors a PR from the GitLab repository, extending the support for
snitch_clusterplatform by adding support for single-buffered tiling and more operators, such as:Added
ExecutionBlock.ExecutionBlock.--debugflag tocargo installwhen installing Banshee to get the possibility of enabling the debug prints.snitch_clusterplatform.main.cto disable printing and testing (convenient when running RTL simulations).Changed
Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.