sw: Add minimal runtime and toolchain file for generic llvm compilation #57

JosseVanDelm · 2023-12-14T18:02:52Z

Add generic LLVM support

This adds an opt-in option for two things that are closely related:

A generic runtime that works with the generic riscv32 target in LLVM (or any other C compiler)
can be invoked with make sw SELECT_RUNTIME=rtl-generic
A toolchain file that selects different versions of LLVM (snitch-specific 12, or generic 17)
can be invoked with make sw SELECT_RUNTIME=rtl-generic SELECT_TOOLCHAIN=llvm-generic

This combination yields a new "support matrix":

support matrix	rtl runtime	rtl-generic runtime
llvm-12 (pulp)	✔️ All custom features supported 🛑 Old LLVM version	✔️ custom instructions supported 🛑 Old LLVM version, drops OpenMP support, drops SSR support, drops data mover support
llvm-17 (generic)	🛑 This combination is not supported	✔️ Newest LLVM version 🛑 No custom instruction support, drops OpenMP support, drops SSR support, drops data mover support

What I had to do to make this happen:

Install LLVM 17 sw: Add upstream LLVM/Clang installation in docker #58
Add a new runtime, introduced in /target/snitch_cluster/sw/runtime/rtl-generic, this is largely the same as /target/snitch_cluster/sw/runtime/rtl, but disables a few imports that mostly require custom builtins, that are not known to generic LLVM. Only snrt.c and snrt.h are adapted. I've kept the comments to make clear what disappeared. Other files literally include their rtl counterparts.
Untangle the build of certain programs. This repo uses a makefile system that calls subsequent makefiles to build all programs. If you choose to build with generic LLVM and the generic runtime, there are a bunch of programs that won't compile anymore. I've included if(n)eq statements that switch on SELECT_RUNTIME and SELECT_TOOLCHAIN parameters, to disable the build of certain programs.
Untangle testing of certain programs. We already introduced a split in support by removing or adding certain cores to our hardware implementations. The new LLVM and runtime now introduce even more split to these tests. The tests are now separated into various yaml files that are logically grouped together. To make it more practical to test multiple files, I added sw: Add support for multiple yaml files in run.py #64
Add two jobs to CI that test SNAX tests with the new runtime and toolchain make sw SELECT_TOOLCHAIN=llvm-generic SELECT_RUNTIME=rtl-generic

In more detail, functionality we lose by going to generic LLVM

Note that we are giving up certain functionality here:

OpenMP support is dropped in the generic runtime, because this has calls to builtins
SSR support is dropped in the generic runtime, because this also calls builtins, although a workaround shouldn't be too difficult, since there already seems to be a path to use generic CSRs instead anyway.
data mover tests are not working anymore.
I'm not sure how these relate to the dma instructions, because those seem to work just fine.
Some mnemonics don't work anymore, because they are not part of any RISC-V standard, and hence are not supported in upstream LLVM (e.g. https://iis-git.ee.ethz.ch/smach/smallFloat-spec), or official extensions have been ratified instead since introduction (Zfhmin, Zfh). This means that many of the computational kernels in the blas and dnn folders don't work anymore, since they contain inline assembly that needs to be known by the compiler.
For non-relocatable instructions it should be fine to replace these with hardcoded .word directives inside the assembly.

To do's

~~Refactor run.yaml to have one for the generic runtime~~ There are now three different files for the above, suggestions welcome!
Refactor toolchain.mk to contain all toolchains instead having two separate files that need to be if-elsed in all different makefiles.
Add tests for both toolchains and runtimes in CI?
Install LLVM toolchain with LLVM.sh
~~refactor generic runtime to not copy-paste all files?~~ I don't know how to do this, currently all files within rtl-generic folder are just copy pasted from the rtl folder.
~~Edit license headers~~ looks fine

JosseVanDelm · 2023-12-15T15:22:19Z

@rgantonio I need a newer version of the container to run these tests.
Check #58

rgantonio · 2023-12-18T07:03:05Z

OMG I just saw this now haha wait haha

rgantonio

Got some questions of course!

Hmm I wonder how to do the /rtl part without copy-pasting hmmm.

rgantonio · 2023-12-18T07:33:55Z

target/snitch_cluster/sw/Makefile

@@ -30,5 +32,5 @@ apps: $(RUNTIME) math $(SNAX_MAC) $(SNAX_GEMM)
 tests: $(RUNTIME) math $(SNAX_MAC) $(SNAX_GEMM)
 	$(MAKE) -C $@ $(TARGET)

-runtime/rtl runtime/banshee math $(SNAX_MAC) $(SNAX_GEMM):
+runtime/rtl runtime/banshee runtime/rtl-generic math $(SNAX_MAC) $(SNAX_GEMM):


Are there dependencies between runtime/rtl and runtime/rtl-generic ?

Maybe we can just do the same $(RUNTIME) for this part.

@rgantonio, no there are none. Actually now that I'm thinking of this, I would actually prefer it if we could change the other targets on this line also to be snax/mac and snax/gemm instead, in that way it is nice and explicit what is being built, but if the name changes then you get a lot of place to change it....
I don't know what the best solution is hahah. Let me know if you still like me to change it

@JosseVanDelm let's change it to the snax/mac and the snax/gemm version. In the latest Snitch Cluster they used it this way :)

@JosseVanDelm maybe change the $(SNAX_MAC) and $(SNAX_GEMM) as well? So it's uniform? Just an idea haha although it's just a minor thing...

rgantonio · 2023-12-18T07:47:21Z

target/snitch_cluster/sw/run-generic.yaml

I know you are not a fan of file duplication haha I wonder if we can actually break the run-yaml tests down separately.

For example, maybe we have:

run-fp.yaml that actually runs all the fp builds only

run-basic.yaml tests things like atomics.elf, barrier.elf, dma_simple.elf and so on.

run-snax-mac.yaml only tests the snax-mac stuff

Then in the ci we can always call multiple run.py tests for the different tests. What do you think?

@rgantonio I was looking at ways to include yaml files into one, but actually just calling run multiple times makes much more sense, will look into this

@JosseVanDelm I was also looking if there was a conditional statement way of doing it in yaml haha but it's not elegant I think. I mean there is but it looked odd...

@rgantonio I've split up the tests and it seems to work now 😄, while also being about 100 lines less!

rgantonio · 2023-12-18T17:39:44Z

@JosseVanDelm since this is WIP, I will ask permission to add/remove stuff just to kick the CI haha

rgantonio · 2023-12-18T22:06:45Z

@JosseVanDelm I fixed the docker thing :) tomorrow I will do the docker build PR for automatic builds.

This reverts commit 03ba20b.

Also appears to be disabled upstream

Not sure if it works with the newest version

These tests require 9 cores (1 data mover + 8 compute)

Some tests require multiple cores to be present

This might just be the ugliest fix ever, but it seems to work

JosseVanDelm · 2023-12-19T17:03:34Z

@rgantonio I went from 391 to 340 lines with the last commit, it's the ugliest way to include the rtl thing, but it works

jorendumoulin

🤯 This is super nice!

I can't notice anything wrong, but I'm not super familiar with the codebase.

It might be useful to include the PR documentation and support matrix to the documentation for future reference. If we add custom support for something in a specific combination of toolchain-runtime, we can keep track of this nicely.

Also the support matrix in a following format looks more clear to me:

Runtime	rtl	rtl-generic
Toolchain	llvm-snitch		llvm-generic
LLVM Version	12 ❌	12 ❌	17 ✔️
Custom Instructions	✔️	✔️	❌
OpenMP Support	✔️	❌	❌
SSR Support	✔️	❌	❌
Data Mover Support	✔️	❌	❌

target/snitch_cluster/sw/dnn.yaml

target/snitch_cluster/sw/toolchain.mk

rgantonio

BYUTIPUL with the current. And maybe also consider some of the comments I made? 😄

Very minor things. I think the testing in CI to build 1 hardware but just change the SW builds should be better so it reduces build time 😄

rgantonio · 2023-12-20T09:06:37Z

.github/workflows/ci.yml

+          ./run.py --simulator verilator \
+          sw/runtime.yaml sw/snitch-cluster-runtime.yaml sw/custom-fp.yaml \
+          sw/standard-fp.yaml sw/openmp.yaml sw/snitch-cluster-openmp.yaml \
+          sw/blas.yaml sw/dnn.yaml -j


  ##############################################


Label to SNAX Cluster w/ Verilator? hehe I think I forgot to change this too.

.github/workflows/ci.yml

rgantonio · 2023-12-20T09:08:56Z

.github/workflows/ci.yml

+          make -C target/snitch_cluster sw \
+          SELECT_RUNTIME=rtl-generic \
+          SELECT_TOOLCHAIN=llvm-generic
+      - name: Run Tests
+        working-directory: target/snitch_cluster
+        run: |-
+          ./run.py --simulator verilator \
+          sw/runtime.yaml sw/standard-fp.yaml sw/snax-gemm-run.yaml -j


Of course, same as above haha

rgantonio · 2023-12-20T09:09:58Z

target/snitch_cluster/sw/Makefile

@@ -30,5 +32,5 @@ apps: $(RUNTIME) math $(SNAX_MAC) $(SNAX_GEMM)
 tests: $(RUNTIME) math $(SNAX_MAC) $(SNAX_GEMM)
 	$(MAKE) -C $@ $(TARGET)

-runtime/rtl runtime/banshee math $(SNAX_MAC) $(SNAX_GEMM):
+runtime/rtl runtime/banshee runtime/rtl-generic math $(SNAX_MAC) $(SNAX_GEMM):


@JosseVanDelm maybe change the $(SNAX_MAC) and $(SNAX_GEMM) as well? So it's uniform? Just an idea haha although it's just a minor thing...

rgantonio · 2023-12-21T06:48:15Z

@JosseVanDelm push?

…on (#57) * Add rtl-generic runtime which compiles with generic llvm * Add generic llvm toolchain and generic toolchain setting * Add LLVM generic toolchain installation to chisel dockerfile * Make makefile work with rtl-generic and clang-12 * Add zfh option and generic llvm examples * Move toolchain selection to toolchain.mk * Make yaml-lint happy * Add back removed files from rtl-generic runtime for clarity * Make lint-yaml even happier * Hardcode an oopsie in toolchain.mk to make old toolchain work * Add tests with generic runtime to CI * Revert "Add LLVM generic toolchain installation to chisel dockerfile" This reverts commit 03ba20b. * Add separate test files * Disable interrupt test Also appears to be disabled upstream * Clean up DNN tests * Hardcode version of clang runtime to be 12.0.1 Not sure if it works with the newest version * Move snitch_cluster specific tests to separate file These tests require 9 cores (1 data mover + 8 compute) * Split openmp tests into snitch-cluster specific ones Some tests require multiple cores to be present * Add right test invocations to snitch-runtime specific ones * Add multiple tests to single runner * Fix -j argument in test runner * Add snitch-specific parts to test build * Reduce rtl runtime code duplication This might just be the ugliest fix ever, but it seems to work * Fix comment in CI workflow * Make makefile more uniform

JosseVanDelm self-assigned this Dec 14, 2023

JosseVanDelm requested review from jorendumoulin, rgantonio and xiaoling-yi December 15, 2023 14:58

JosseVanDelm mentioned this pull request Dec 15, 2023

sw: Add upstream LLVM/Clang installation in docker #58

Merged

JosseVanDelm changed the title ~~Add minimal runtime and toolchain file for generic llvm compilation~~ sw: Add minimal runtime and toolchain file for generic llvm compilation Dec 15, 2023

rgantonio reviewed Dec 18, 2023

View reviewed changes

JosseVanDelm force-pushed the Josse/add-minimal-runtime branch from 7d4a16f to bd733e4 Compare December 18, 2023 15:38

JosseVanDelm mentioned this pull request Dec 19, 2023

sw: Add support for multiple yaml files in run.py #64

Merged

JosseVanDelm added 17 commits December 19, 2023 15:38

Add rtl-generic runtime which compiles with generic llvm

3904dd4

Add generic llvm toolchain and generic toolchain setting

bc79067

Add LLVM generic toolchain installation to chisel dockerfile

544877d

Make makefile work with rtl-generic and clang-12

cf2f6a8

Add zfh option and generic llvm examples

3a29ddc

Move toolchain selection to toolchain.mk

6e55460

Make yaml-lint happy

4a75791

Add back removed files from rtl-generic runtime for clarity

b63006b

Make lint-yaml even happier

782953c

Hardcode an oopsie in toolchain.mk to make old toolchain work

d5eef28

Add tests with generic runtime to CI

52df79a

Revert "Add LLVM generic toolchain installation to chisel dockerfile"

d778556

This reverts commit 03ba20b.

Add separate test files

7de5efa

Disable interrupt test

1074f8a

Also appears to be disabled upstream

Clean up DNN tests

e1fcb36

Hardcode version of clang runtime to be 12.0.1

864c7a1

Not sure if it works with the newest version

Move snitch_cluster specific tests to separate file

263b3ad

These tests require 9 cores (1 data mover + 8 compute)

JosseVanDelm added 4 commits December 19, 2023 15:38

Split openmp tests into snitch-cluster specific ones

1bfe7bd

Some tests require multiple cores to be present

Add right test invocations to snitch-runtime specific ones

5928611

Add multiple tests to single runner

809000b

Fix -j argument in test runner

016e202

JosseVanDelm force-pushed the Josse/add-minimal-runtime branch from 3908f36 to 016e202 Compare December 19, 2023 14:46

JosseVanDelm added 2 commits December 19, 2023 16:35

Add snitch-specific parts to test build

a533aa8

Reduce rtl runtime code duplication

544faef

This might just be the ugliest fix ever, but it seems to work

JosseVanDelm marked this pull request as ready for review December 19, 2023 17:23

JosseVanDelm requested a review from rgantonio December 19, 2023 17:24

jorendumoulin approved these changes Dec 20, 2023

View reviewed changes

target/snitch_cluster/sw/dnn.yaml Show resolved Hide resolved

target/snitch_cluster/sw/toolchain.mk Show resolved Hide resolved

rgantonio approved these changes Dec 20, 2023

View reviewed changes

JosseVanDelm mentioned this pull request Dec 21, 2023

Look into libc linking #66

Open

Fix comment in CI workflow

5f46002

JosseVanDelm mentioned this pull request Dec 21, 2023

Reuse hardware build for multiple software tests? #67

Closed

Make makefile more uniform

5bc299d

This was referenced Dec 21, 2023

ci: Move sw builds before hw build #68

Merged

Add generic runtime documentation #69

Closed

JosseVanDelm merged commit d87720f into main Dec 21, 2023
17 checks passed

This was referenced Dec 21, 2023

Tracking issue: Move to generic runtime KULeuven-MICAS/snax-mlir#48

Closed

Use upstream LLVM with generic runtime KULeuven-MICAS/snax-mlir#49

Merged

JosseVanDelm deleted the Josse/add-minimal-runtime branch December 22, 2023 14:13

JosseVanDelm mentioned this pull request Jan 8, 2024

Add SNAX to this repository pulp-platform/snitch_cluster#77

Closed

JosseVanDelm mentioned this pull request Aug 27, 2024

docker: use pre-built mlir and cva6 toolchain #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sw: Add minimal runtime and toolchain file for generic llvm compilation #57

sw: Add minimal runtime and toolchain file for generic llvm compilation #57

JosseVanDelm commented Dec 14, 2023 •

edited

Loading

JosseVanDelm commented Dec 15, 2023

rgantonio commented Dec 18, 2023

rgantonio left a comment

rgantonio Dec 18, 2023

JosseVanDelm Dec 18, 2023

rgantonio Dec 18, 2023

rgantonio Dec 20, 2023

rgantonio Dec 18, 2023

JosseVanDelm Dec 18, 2023

rgantonio Dec 18, 2023

JosseVanDelm Dec 19, 2023

rgantonio commented Dec 18, 2023

rgantonio commented Dec 18, 2023

JosseVanDelm commented Dec 19, 2023

jorendumoulin left a comment

rgantonio left a comment

rgantonio Dec 20, 2023

rgantonio Dec 20, 2023

rgantonio Dec 20, 2023

rgantonio commented Dec 21, 2023

sw: Add minimal runtime and toolchain file for generic llvm compilation #57

sw: Add minimal runtime and toolchain file for generic llvm compilation #57

Conversation

JosseVanDelm commented Dec 14, 2023 • edited Loading

Add generic LLVM support

What I had to do to make this happen:

In more detail, functionality we lose by going to generic LLVM

JosseVanDelm commented Dec 15, 2023

rgantonio commented Dec 18, 2023

rgantonio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgantonio commented Dec 18, 2023

rgantonio commented Dec 18, 2023

JosseVanDelm commented Dec 19, 2023

jorendumoulin left a comment

Choose a reason for hiding this comment

rgantonio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgantonio commented Dec 21, 2023

JosseVanDelm commented Dec 14, 2023 •

edited

Loading