Update boost deps for core and mss, track Sparta PR397 #42

peter-d · 2023-03-13T18:49:50Z

This fixes #35 and tracks the changes in Sparta PR397

…ludes

…istics

…parta

…write

ghost · 2023-03-13T20:54:46Z

Ok, step 1 complete: MAP PR merged: sparcians/map#397

ghost · 2023-03-13T20:54:59Z

Now to get this regressing!

peter-d · 2023-03-27T10:30:38Z

@knute-sifive any objections if we move cmake version required forward to 3.19 or more recent? Right now we use 3.17 here, but that does not yet define the catch-all HDF5::HDF5 target, which was introduced in 3.19. And that breaks the build here. For reference: Sparta uses 3.26 in CI, but the cmake files say min = 3.15, so that's also wrong (I'll file a quick PR for that in spara repo as well)

Over here, I'll update the conda environment file with a more recent version and push that, but I am not sure if this will automatically update the image on which CI runs, I might need your help for that.

peter-d · 2023-03-27T11:56:40Z

@knute-sifive I'm kind of stuck at this point: I have the HDF5 issue fixed, but with the CI pinned to sparta v1.1.1 this won't be able to pass...

Any advice? I don't want to break master CI either.

Also the conda environment.yml had everything pinned to exact versions, and pushing cmake to a newer version created an avalanche of dependent version changes. I've changed all =version lines to >=version. So this might have some unintended side effects too...

peter-d · 2023-03-28T19:15:20Z

Glad you are looking into that - that’s too much magic for me ;)
Thanks

ghost · 2023-03-28T19:40:34Z

Huzzah!

ghost · 2023-03-28T19:43:24Z

Hey Peter, update your clone and see if a release and/or debug build works for you.

peter-d · 2023-03-29T06:42:32Z

core/CMakeLists.txt

@@ -20,5 +20,3 @@ add_library(core
  CPUFactory.cpp
  CPUTopology.cpp
  )
-target_include_directories(core SYSTEM PRIVATE ${Boost_INCLUDE_DIRS})
-target_link_libraries(core PUBLIC SPARTA::sparta)


Since I don't do a conda install of sparta, this fails for me: sparta include paths and link paths are not passed properly.

peter-d · 2023-03-29T07:52:57Z

I had to put back a few target_link_libraries to find sparta (I don't have it installed in conda env so it's not picked up automatically in the default paths).

Since that does not break CI, this looks good to go to me, but it could do with a review especially to check:

is it ok to pull in mavis and stf submodules from current masters? I updated the references
the environment.yml file was coming from a conda list but I don't run in conda, so I manually edited the file. It currently works, but maybe is not what we want.

ghost · 2023-03-29T15:10:48Z

I had to put back a few target_link_libraries to find sparta

That's so weird that's required for the core library build. Curious -- what didn't link? The issue I have with adding sparta as a link library is that I believe this causes the core archive to blow up in size. I think.

is it ok to pull in mavis and stf submodules from current masters?

I don't see problem with that. Probably should use an stf tag and tag Mavis so we have consistency and not some random SHA. I'll look into that on the next revision.

Updating the yaml file for Conda is fine -- the CI uses it, so if it passes, then it should good to go!

ghost · 2023-03-29T15:10:58Z

I had to put back a few target_link_libraries to find sparta

That's so weird that's required for the core library build. Curious -- what didn't link? The issue I have with adding sparta as a link library is that I believe this causes the core archive to blow up in size. I think.

is it ok to pull in mavis and stf submodules from current masters?

I don't see problem with that. Probably should use an stf tag and tag Mavis so we have consistency and not some random SHA. I'll look into that on the next revision.

Updating the yaml file for Conda is fine -- the CI uses it, so if it passes, then it should good to go!

ghost · 2023-03-29T15:11:19Z

I had to put back a few target_link_libraries to find sparta

That's so weird that's required for the core library build. Curious -- what didn't link? The issue I have with adding sparta as a link library is that I believe this causes the core archive to blow up in size. I think.

is it ok to pull in mavis and stf submodules from current masters?

I don't see problem with that. Probably should use an stf tag and tag Mavis so we have consistency and not some random SHA. I'll look into that on the next revision.

Updating the yaml file for Conda is fine -- the CI uses it, so if it passes, then it should good to go!

peter-d · 2023-03-29T19:14:48Z

I had to put back a few target_link_libraries to find sparta

That's so weird that's required for the core library build. Curious -- what didn't link? The issue I have with adding sparta as a link library is that I believe this causes the core archive to blow up in size. I think.

But don't all those core units depend on Sparta? I get the errors during compile on missing includes, not on linking:

In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Inst.cpp:3:
/Users/pdback/src/peter-d/riscv-perf-model/core/Inst.hpp:5:10: fatal error: 'sparta/memory/AddressTypes.hpp' file not found
#include "sparta/memory/AddressTypes.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Execute.cpp:3:
/Users/pdback/src/peter-d/riscv-perf-model/core/Execute.hpp:4:10: fatal error: 'sparta/simulation/Unit.hpp' file not found
#include "sparta/simulation/Unit.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/CPUFactory.cpp:4:
/Users/pdback/src/peter-d/riscv-perf-model/core/CPUFactory.hpp:6:10: fatal error: 'sparta/simulation/ResourceFactory.hpp' file not found
#include "sparta/simulation/ResourceFactory.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Fetch.cpp:9:
/Users/pdback/src/peter-d/riscv-perf-model/core/Fetch.hpp:12:10: fatal error: 'sparta/ports/DataPort.hpp' file not found
#include "sparta/ports/DataPort.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Decode.cpp:6:
/Users/pdback/src/peter-d/riscv-perf-model/core/Decode.hpp:8:10: fatal error: 'sparta/ports/DataPort.hpp' file not found
#include "sparta/ports/DataPort.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Preloader.cpp:2:
/Users/pdback/src/peter-d/riscv-perf-model/core/Preloader.hpp:5:10: fatal error: 'sparta/simulation/Resource.hpp' file not found
#include "sparta/simulation/Resource.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/pdback/src/peter-d/riscv-perf-model/core/LSU.cpp:2:10: fatal error: 'sparta/utils/SpartaAssert.hpp' file not found
#include "sparta/utils/SpartaAssert.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Rename.cpp:9:
/Users/pdback/src/peter-d/riscv-perf-model/core/Rename.hpp:8:10: fatal error: 'sparta/ports/DataPort.hpp' file not found
#include "sparta/ports/DataPort.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/ROB.cpp:5:
/Users/pdback/src/peter-d/riscv-perf-model/core/ROB.hpp:7:10: fatal error: 'sparta/ports/DataPort.hpp' file not found
#include "sparta/ports/DataPort.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~

peter-d · 2023-03-29T19:18:31Z

I added target_link_libraries(... SPARTA::sparta) since that then automatically also sets the right include paths for sparta when compiling. But I did not really consider the impact it might have at linking stage...

Since it's all statically linked at the end, I guess it could be sufficient to just add the includes here?

ghost · 2023-03-29T23:38:25Z

CMakeLists.txt

+else()
+  message (STATUS "Sparta was found in ${SPARTA_SEARCH_DIR}")
+endif()
+#include_directories(${SPARTA_INCLUDE_DIRS})


I'm wondering if uncommenting this line will fix your include issues. Can you do that AND remove the target_link lines to see if you still have problems?

It helps to some extent: No more errors on Sparta includes, but it lacks the secondary includes it seems, as now I am getting boost errors: (which on my system is also not in a default system path, because I am pointing to a specific version somewhere in /opt/homebrew)

(which ironically is back to square one - not finding boost was my initial problem to tackle all makefiles ;-) )

In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Decode.cpp:6: In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Decode.hpp:8: In file included from /Users/pdback/opt/sparta/include/sparta/ports/DataPort.hpp:15: In file included from /Users/pdback/opt/sparta/include/sparta/ports/Port.hpp:19: In file included from /Users/pdback/opt/sparta/include/sparta/events/Event.hpp:15: In file included from /Users/pdback/opt/sparta/include/sparta/events/EventNode.hpp:16: In file included from /Users/pdback/opt/sparta/include/sparta/simulation/Clock.hpp:21: /Users/pdback/opt/sparta/include/sparta/kernel/Scheduler.hpp:13:10: fatal error: 'boost/timer/timer.hpp' file not found #include <boost/timer/timer.hpp> ^~~~~~~~~~~~~~~~~~~~~~~

It's what this line and the next in FindSparta.cmake are providing automatically when you add sparta as a link library:

https://github.com/sparcians/map/blob/6f0636f7a717238cd039be33f7975048449bb82c/sparta/cmake/FindSparta.cmake#L69

Ok, I can reproduce the missing include problem, but the not boost one. I'll admit that I don't fully grok how cmake brings in the goods from a package. You'd think the find_package method would be enough! But sparta must be installed in a "standard" location or something so that includes AND library paths are set up? I don't get it...

All find_package does is import a target for the library (as if it were defined elsewhere in the makefiles with add_library) but it won’t set up any default compile or link flags project wide, it will only add the relevant options to the targets when you explicitly say they depend on the imported target. In this case that means unless you explicitly add SPARTA::sparta it won’t pass the flags. Which you won’t notice if everything is installed in system wide include and lib paths, but it will break as soon as one of the components (either Sparta or the lobs it depends on) is not in that default path.

That said, there could be a way to only add the include paths and maybe not the link flags, but I expect that to fail at linking still…

Reason I see boost problems is because I have multiple boost versions via homebrew, and Sparta needs one that is not the default one. (There’s macOS issues with boost 1.81 command line arguments)

Thank you Peter and Knute for working through this.

Trying to understand the latest status of this work: From the comments above, I understand at least the regression is not working yet. But is it just the regression or more than that? Would I be able to build riscv-perf-model / olympia with latest head of master of Sparta (which installs Sparta in the conda environment)?

Actually regression is working with these changes, but only because the CI is strictly controlled:

Create a conda environment

Build and regress everything inside that conda environment

For those folks that do not use conda or prefer not to, it's hard to set up a CMakeList flow that works for both.

As long as Sparta (and it’s dependencies like boost hdf5 etc) are available in the default paths (libs and includes) it just works. It’s just in case some dependencies are not in a default location, that the overridden paths are not passed through from Sparta to Olympia… (eg I have a home brew boost version in macOS that is not the default one which gives me troubles when I want to this locally branch)

peter-d · 2023-04-11T11:05:39Z

@knute-sifive so just so I fully get this what's the intended goal in the end? And why is it not ok to tell cmake that Olympia links against sparta?

As far as I understand:

libsparta.a will be statically linked into Olympia. But libsparta.a does depend on a bunch of third party libs (boost, hdf5, etc) which will be dynamically linked to in some cases, or are header only in some other cases.

In order to be able to compile Olympia objects that use Sparta, the include paths to Sparta need to be set up correctly, of course. Possibly also those of dependent libs? (I haven't checked if Sparta header files include header files from its dependencies, but it's likely at least some do)

Then at link time, we need to point Olympia to the Sparta static lib libsparta.a but also to the right libraries Sparta links against?

Then I don't really see why it would be bad to just include the target_link_libraries(... SPARTA::sparta) to set up both include as well as link command line switches for Sparta and its dependencies properly. But then again, I might be missing something here ;)

ghost · 2023-04-11T20:27:20Z

Then I don't really see why it would be bad to just include the target_link_libraries(... SPARTA::sparta) to set up both include as well as link command line switches for Sparta and its dependencies properly. But then again, I might be missing something here ;)

So, adding target_link_libraries to every library in the build will add libsparta.a and libsimdb.a into those archives, which just bloats them. What you want is the include paths to be set up and that's all.

I was stuck on this for the past week until I chatted with @bdutro about what we think cmake might be doing here.

I think we understand now.

In the main CMakeList.txt, there's this line:

target_link_libraries (olympia core mss SPARTA::sparta ${STF_LINK_LIBS})

which brings in Sparta libraries and includes for just Olympia and not the libraries.

However, I think if you add this line to the top level CMakeList.txt, all should work:

include_directories (SYSTEM ${SPARTA_INCLUDE_DIRS})

Can you try this (and remove the target_link_libraries from core)?

peter-d · 2023-04-28T12:29:49Z

So this didn't work right away and I haven't had time to check this further until today.
There were a few things going on:

we need to check with the found sparta where it's dependencies are, I've done that in core and mss CMakeLists.txt
But then it turns sparta is not exporting the right includes in its property (the ones you need to parse the header files), which I only noticed once the target_link_libraries statement was removed, as they are all dragged in properly by that link command.

Which means there's really not much that needs changing here, but I'll need to fix the FindSparta.cmake once more.

Signed-off-by: Peter Debacker <[email protected]>

peter-d · 2023-04-28T13:06:01Z

@knute-sifive if you could review?
For me this is good to go (if debug CI won't fail for a weird reason)
Thanks!

ghost · 2023-04-29T01:51:25Z

Absolutely! Yes, let's get this in by the weekend.

.github/workflows/ubuntu-build.yml

Updated with map_v2.0 notes Signed-off-by: Knute Lingaard <[email protected]>

Peter Debacker and others added 8 commits February 3, 2023 14:04

tell cmake to include boost libs

2cd2608

fixed boost include for tests

6299f22

using the FindSparta.cmake which sparta installs to find libs and inc…

4795d0b

…ludes

Merge branch 'riscv-software-src:master' into boost_fix

828d884

tracking sparta PR #408, weightedcontextcounter is now in sparta/stat…

88d0200

…istics

CI should pick up sparta in default location after make install for s…

86d45bf

…parta

install sparta in github workspace area where we have permissions to …

eb4182c

…write

no need to mark project as C, hdf5 find is fixed

7fdb723

peter-d mentioned this pull request Mar 13, 2023

Knutel/peter d cmake include fixes sparcians/map#397

Merged

Resolved merge conflict with master

8ce74e9

ghost assigned peter-d Mar 16, 2023

ghost added the enhancement New feature or request label Mar 16, 2023

peter-d mentioned this pull request Mar 27, 2023

Bump cmake version requirement to 3.19 sparcians/map#414

Closed

peter-d added 5 commits March 27, 2023 13:12

bump cmake to more recent version, so it can find hdf5

5722d2a

somewhat less aggressive: 3.19 might be ok?

783865b

need existing version

24bc16d

nuclear option: no version pins at all

1194158

branch needs sparta master

e615d9e

peter-d added 8 commits March 28, 2023 09:37

double check on sparta git version we use in CI

57aadb2

hacky workaround to get the right sparta branch in CI

f23e0ec

hacky workaround to get the right sparta branch in CI, v2

6fd31f2

hacky workaround to get the right sparta branch in CI, v3

60dcb1f

hacky workaround to get the right sparta branch in CI, v4

d933500

Merge branch 'master' into boost_fix

0667094

latest submodules just like master

aa8d97e

verbose build for CI debug

85f9c55

Setup linker support for LTO

fdc60b3

peter-d commented Mar 29, 2023

View reviewed changes

add in link libraries where they were missing

a7cf25d

ghost reviewed Mar 29, 2023

View reviewed changes

Knute Lingaard and others added 3 commits April 13, 2023 19:45

I think these changes will work in Peter's environment

53cac69

Added cool banner

f3d7ecc

get all required includes for sparta from the property

f1c16b3

peter-d and others added 3 commits April 28, 2023 14:31

resolve conflict with master

1123995

Merge branch 'master' into boost_fix

80e2b30

Signed-off-by: Peter Debacker <[email protected]>

should not have removed install

589c16d

peter-d commented May 3, 2023

View reviewed changes

.github/workflows/ubuntu-build.yml Outdated Show resolved Hide resolved

klingaard and others added 2 commits May 7, 2023 17:14

Use the branch map_v2 for regressions

faed611

Update README.md

5ce1b6a

Updated with map_v2.0 notes Signed-off-by: Knute Lingaard <[email protected]>

ghost approved these changes May 7, 2023

View reviewed changes

peter-d merged commit 32fc79d into master May 8, 2023

peter-d deleted the boost_fix branch May 8, 2023 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update boost deps for core and mss, track Sparta PR397 #42

Update boost deps for core and mss, track Sparta PR397 #42

peter-d commented Mar 13, 2023

ghost commented Mar 13, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 27, 2023

peter-d commented Mar 27, 2023 •

edited

Loading

peter-d commented Mar 28, 2023

ghost commented Mar 28, 2023 •

edited by ghost

Loading

ghost commented Mar 28, 2023

peter-d Mar 29, 2023

peter-d commented Mar 29, 2023

ghost commented Mar 29, 2023

ghost commented Mar 29, 2023

ghost commented Mar 29, 2023

peter-d commented Mar 29, 2023

peter-d commented Mar 29, 2023

ghost Mar 29, 2023 •

edited by ghost

Loading

peter-d Mar 30, 2023

peter-d Mar 30, 2023

ghost Mar 31, 2023

peter-d Apr 1, 2023 •

edited

Loading

arupc Apr 5, 2023

ghost Apr 5, 2023

peter-d Apr 5, 2023 •

edited

Loading

peter-d commented Apr 11, 2023

ghost commented Apr 11, 2023

peter-d commented Apr 28, 2023

peter-d commented Apr 28, 2023

ghost commented Apr 29, 2023

Update boost deps for core and mss, track Sparta PR397 #42

Update boost deps for core and mss, track Sparta PR397 #42

Conversation

peter-d commented Mar 13, 2023

ghost commented Mar 13, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 27, 2023

peter-d commented Mar 27, 2023 • edited Loading

peter-d commented Mar 28, 2023

ghost commented Mar 28, 2023 • edited by ghost Loading

ghost commented Mar 28, 2023

peter-d Mar 29, 2023

Choose a reason for hiding this comment

peter-d commented Mar 29, 2023

ghost commented Mar 29, 2023

ghost commented Mar 29, 2023

ghost commented Mar 29, 2023

peter-d commented Mar 29, 2023

peter-d commented Mar 29, 2023

ghost Mar 29, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

peter-d Mar 30, 2023

Choose a reason for hiding this comment

peter-d Mar 30, 2023

Choose a reason for hiding this comment

ghost Mar 31, 2023

Choose a reason for hiding this comment

peter-d Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

arupc Apr 5, 2023

Choose a reason for hiding this comment

ghost Apr 5, 2023

Choose a reason for hiding this comment

peter-d Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

peter-d commented Apr 11, 2023

ghost commented Apr 11, 2023

peter-d commented Apr 28, 2023

peter-d commented Apr 28, 2023

ghost commented Apr 29, 2023

peter-d commented Mar 27, 2023 •

edited

Loading

ghost commented Mar 28, 2023 •

edited by ghost

Loading

ghost Mar 29, 2023 •

edited by ghost

Loading

peter-d Apr 1, 2023 •

edited

Loading

peter-d Apr 5, 2023 •

edited

Loading