Build system assumes MURISC-V NN is built as a top-level project

[tomhepworth]

In Tests/CMakeLists.txt there is:

target_include_directories(unity PUBLIC $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Tests/TestData>
                                        $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/Tests/Utils>
                                        $<BUILD_INTERFACE:${unity_SOURCE_DIR}/src>)

Which uses references to CMAKE_SOURCE_DIR. This is fine as long as the top level CMakeLists.txt file is in the top level of muriscv-nn, however if someone (like me :) ) wants to use this project as a library then a common way to do things would be having a project set up as follows:

project/
├─ CMakeLists.txt <-- (my own cmakelists)
├─ libs/
│  ├─ muriscv-nn/
│  │  ├─ CMakeLists.txt <-- (your cmakelists)
│  │  ├─ ...
├─ src/
│  ├─ my_source_code.c

muriscv-nn will be added to the project using add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR/libs/mursicv-nn}) in my top level CMake, and linked against my own target.

In this case $CMAKE_SOURCE_DIR always points to my top level CMakeLists.txt, and so all paths relative to CMAKE_SOURCE_DIR inside the murisc-v library break. This is exactly what is described here: https://lesleylai.info/en/cmake_src_directory/

Instead PROJECT_SOURCE_DIR or paths relative to CURRENT_SOURCE_DIR should be used.

I am happy to make these changes if required.

[PhilippvK]

@tomhepworth Thank you for reporting this!

I would not consider this as a bug as we do not expect anyone to add the top level directory of the project as CMake subdirectory (as this includes toolchain specifics and testing) which is usually not wanted when building muriscvnn as a 3rd library.

The recommended approach (just adding muriscv-nn/Source) for what you are trying to do is:

SET(MURISCVNN_LIB muriscvnn)
ADD_SUBDIRECTORY(${MURISCVNN_DIR}/Source muriscvnn)
target_include_directories(${MURISCVNN_LIB} PUBLIC
    ${MURISCVNN_DIR}/Include
    ${MURISCVNN_DIR}/Include/CMSIS/NN/Include
)
target_link_libraries(${MURISCVNN_LIB} PUBLIC m)

I will make sure that this information is properly documented in the README.

Your argument that PROJECT_SOURCE_DIR is a better choice than CMAKE_SOURCE_DIR is still valid. Hence I will look into the related PR soon.

[tomhepworth]

Thank you for the reply :)

In my case I would benefit from the ability to run tests as well. I plan on tweaking some functions as an experiment and I would like to ensure that that I still match the CMSIS behaviour. I suppose I could tweak and test a copy of the repo in a separate directory, and only move changes across once they work, but it seems a lot simpler to have everything in the same place.
I guess I could always build/test muriscv-nn separately, but it is quite a nice "workflow" to just git submodule add http://...muriscv-nn.git and then add_subdirectory(muriscv-nn) and not have to worry about it.

After my PR I think both methods would work?

[tomhepworth]

@PhilippvK

The recommended approach you describe still has some issues for me

For exampel ${SIMULATOR} seems to be set in muriscv-nn/CMakeLists.txt but gets used in muriscv-nn/Source/CMakeLists.txt which seems to cause:

CMake Error at libs/muriscv-nn/Source/CMakeLists.txt:44 (if):
  if given arguments:

    "STREQUAL" "Vicuna"

  Unknown arguments specified

as the variable has no value when the library is added as you describe.

Commenting out this part leads to

CMake Error at CMakeLists.txt:37 (target_link_libraries):
  Attempt to add link library "m" to target "muriscvnn" which is not built in
  this directory.

  This is allowed only when policy CMP0079 is set to NEW.

So you may need to update the cmake minimum required version to 3.13 https://cmake.org/cmake/help/latest/policy/CMP0079.html

[PhilippvK]

This should be easy to fix. I try to provide a patch ASAP.

[PhilippvK]

@tomhepworth Fixes should be on master. Please let me know if there are still issues integrating muRISCV-NN or close the Issue in case it works as expected.

[tomhepworth]

I am still having a few problems getting it to play nicely with cmake toolchains. I have made my own toolchain file for muriscv-nn which works in isolation but fails when the project is added as a subproject.

Is there a specific way you suggest for adding the library as a subproject and changing the toolchain? I have just set the TOOLCHAIN variable and pointed my wider project at the same toolchain file

[PhilippvK]

@tomhepworth Hi. I am refactoring our toolchain handling anyways these days, so I am happy to look into this. I need some time to reproduce your issue, in case you habe some error message (or a minimal example) that would would help me to figure out what’s going on in your setup.

[tomhepworth]

I think I will be able to hack around it - if you are refactoring then dont worry.

For my purposes ( I work for Codasip btw ) it would be best if there was just some minimal set of variables I could set in some existing toolchain file, and the the library inherits what it needs to from there. EG I have my own simulator and so I edited the conditions to set the simulator command to work with our internal sims, this worked but it would be nice to control everything from a top level.

[tomhepworth]

Sorry, pressed "close" instead of "comment" haha

[PhilippvK]

Sorry for the delays, I was working on getting the recent CMSIS-NN changes synced to get a 100% passing unit/integration tests in the CI. I also created an first official release v0.1.0 and added a (work-in-progress) COMPATIBILITY.md to track what versions of dependencies are compatible with certain releases of muRISCV-NN.

Next steps are:

• [ ] Refresh Benchmark Results Page
• [ ] Move to GCC14 & LLVM 19 (#59)
• [ ] Refactor CMake Toolchain Handling
• [ ] Support RV64 (#60 & #73)
• [ ] Support Zve32x (#61)
• [ ] Update Benchmarks to also run on RV64 and Zve32x

I hope that I can finish these tasks until the end of the month and include them in Release v0.2.0...

[tomhepworth]

Thanks, I appreicate you getting back to me. However just FYI in case it impacts your timeline - I no longer work for Codasip and so these changes will not affect me any more.