-
Notifications
You must be signed in to change notification settings - Fork 647
Allow ChiselSim to link in prebuilt software libraries #5189
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -338,6 +338,39 @@ trait Chisel extends CrossSbtModule with HasScala2MacroAnno with HasScalaPlugin | |
| override def scalacOptions = Task { super.scalacOptions() :+ "-Wconf:cat=other-implicit-type:s" } | ||
|
|
||
| override def testForkGrouping = discoveredTestClasses().grouped(8).toSeq | ||
|
|
||
| // Compile shared libraries for the simulator link library tests. | ||
| def sharedTestLibs: T[Seq[os.Path]] = Task(persistent = true) { | ||
| val srcDir = this.moduleDir / "src" / "test" / "resources" / "chisel3" / "simulator" | ||
| val cc = sys.env.getOrElse("CC", "cc") | ||
| val shared = if (scala.util.Properties.isMac) "-dynamiclib" else "-shared" | ||
| val srcFiles = Seq("linkLibA.c", "linkLibB.c", "linkLibC.c") | ||
| val libFiles = srcFiles.map { src => | ||
| val lib = Task.dest / (src.stripSuffix(".c") + ".so") | ||
| os.proc(cc, shared, "-fPIC", srcDir / src, "-o", lib).call() | ||
| lib | ||
| } | ||
| libFiles | ||
| } | ||
|
|
||
| // Pass some of the pre-compiled shared libraries to the simulator via | ||
| // environment variables. | ||
| override def forkEnv = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkEnv() ++ Map( | ||
| // CHISELSIM_LIBS lets ChiselSim resolve libraries by name: | ||
| "CHISELSIM_LIBS" -> s"linkLibA=${libs(0)}", | ||
| // Also pass in a full path to test the `libraryPaths` setting: | ||
| "LINKLIBC_FULL_PATH" -> libs(2).toString | ||
| ) | ||
| } | ||
|
|
||
| // Pass one of the pre-compiled shared libraries to the simulator via a JVM | ||
| // property. | ||
| override def forkArgs = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkArgs() :+ s"-Dchiselsim.libraries=linkLibB=${libs(1)}" | ||
| } | ||
| } | ||
|
Comment on lines
+358
to
374
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not going to block but I don't love this meaning given to the various libs by index--I'd rather we used a case class (or even named tuple, <3 Scala 3) to give names to them, but I'll let it go. |
||
|
|
||
| def unitTest( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -262,6 +262,41 @@ trait Simulator[T <: Backend] { | |||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| Files.walkFileTree(Paths.get(workspace.primarySourcesPath), new DirectoryFinder) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Collect a list of library names and paths from the environment. These | ||||||||||||||||||||||||||||||||
| // will be used to resolve library names provided by the user. Environment | ||||||||||||||||||||||||||||||||
| // variables take precedence over Java properties. | ||||||||||||||||||||||||||||||||
| def parseLibraryMap(value: Option[String]): Map[String, String] = { | ||||||||||||||||||||||||||||||||
fabianschuiki marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
| value | ||||||||||||||||||||||||||||||||
| .getOrElse("") | ||||||||||||||||||||||||||||||||
| .split(":") | ||||||||||||||||||||||||||||||||
| .filter(_.nonEmpty) | ||||||||||||||||||||||||||||||||
| .map { entry => | ||||||||||||||||||||||||||||||||
| entry.split("=", 2) match { | ||||||||||||||||||||||||||||||||
| case Array(name, path) => name -> path | ||||||||||||||||||||||||||||||||
| case _ => | ||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException( | ||||||||||||||||||||||||||||||||
| s"Invalid link library mapping `$entry`; expected `<name>=<path>`" | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+274
to
+279
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine but bonus points if you give an error with the bad input and a carat, e.g. chisel/src/main/scala/chisel3/stage/ChiselAnnotations.scala Lines 97 to 111 in fad1f6c
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| .toMap | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| val libraryMapFromProp = parseLibraryMap(sys.props.get("chiselsim.libraries")) | ||||||||||||||||||||||||||||||||
| val libraryMapFromEnv = parseLibraryMap(sys.env.get("CHISELSIM_LIBS")) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+284
to
+285
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually these things are either or (and also either-or with being passed as arguments). For example, you can set the I'm not sure if we should do that here, but maybe? What do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know about the classpath behavior. My gut feeling is that since these are lists, we might as well combine them instead of having a single library provided via one mechanism effectively remove the ones provided in the other. That feels somewhat unexpected from a user point of view.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intuition remains that overriding is more likely to be expected by users, but I'm also not going to sandbag on that because I agree that concatenation also feels reasonable. |
||||||||||||||||||||||||||||||||
| val libraryMap = libraryMapFromProp ++ libraryMapFromEnv | ||||||||||||||||||||||||||||||||
| val resolvedLibraryPaths = settings.libraries.map { name => | ||||||||||||||||||||||||||||||||
| libraryMap.getOrElse( | ||||||||||||||||||||||||||||||||
| name, | ||||||||||||||||||||||||||||||||
| throw new NoSuchElementException( | ||||||||||||||||||||||||||||||||
| s"Link library `$name` not found. " + | ||||||||||||||||||||||||||||||||
| "Set the `CHISELSIM_LIBS` environment variable or the " + | ||||||||||||||||||||||||||||||||
| "`chiselsim.libraries` Java property to provide a list of " + | ||||||||||||||||||||||||||||||||
| "`<name>=<path>` mappings separated by `:`. Available libraries: " + | ||||||||||||||||||||||||||||||||
| s"[${libraryMap.keys.mkString(", ")}]" | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| val commonCompilationSettingsUpdated = commonSettingsModifications( | ||||||||||||||||||||||||||||||||
| commonCompilationSettings.copy( | ||||||||||||||||||||||||||||||||
| // Append to the include directorires based on what the | ||||||||||||||||||||||||||||||||
|
|
@@ -275,6 +310,7 @@ trait Simulator[T <: Backend] { | |||||||||||||||||||||||||||||||
| directoryFilter = commonCompilationSettings.directoryFilter.orElse( | ||||||||||||||||||||||||||||||||
| settings.verilogLayers.shouldIncludeDirectory(elaboratedModule, workspace.primarySourcesPath) | ||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||
| linkLibraryPaths = commonCompilationSettings.linkLibraryPaths ++ resolvedLibraryPaths ++ settings.libraryPaths, | ||||||||||||||||||||||||||||||||
| simulationSettings = commonCompilationSettings.simulationSettings.copy( | ||||||||||||||||||||||||||||||||
| plusArgs = commonCompilationSettings.simulationSettings.plusArgs ++ settings.plusArgs, | ||||||||||||||||||||||||||||||||
| enableWavesAtTimeZero = | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| void magicFuncA(int *result) { *result = 42; } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| void magicFuncB(int *result) { *result = 1337; } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| void magicFuncC(int *result) { *result = 9001; } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package svsim | ||
|
|
||
| import com.lihaoyi.unroll | ||
| import java.io.File | ||
| import scala.util.matching.Regex | ||
|
|
||
|
|
@@ -68,7 +69,9 @@ case class CommonCompilationSettings( | |
| includeDirs: Option[Seq[String]] = None, | ||
| fileFilter: PartialFunction[File, Boolean] = PartialFunction.empty, | ||
| directoryFilter: PartialFunction[File, Boolean] = PartialFunction.empty, | ||
| simulationSettings: CommonSimulationSettings = CommonSimulationSettings.default | ||
| simulationSettings: CommonSimulationSettings = CommonSimulationSettings.default, | ||
| // @unroll is required to maintain binary compatibility | ||
| @unroll linkLibraryPaths: Seq[String] = Seq.empty | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the greatest discovery of my life. |
||
| ) | ||
| object CommonCompilationSettings { | ||
| object VerilogPreprocessorDefine { | ||
|
|
||
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.
I'm wondering if instead of applying this to all chisel tests, we shouldn't add a new test build unit for this. Obviously they aren't harmful, but it feels a bit incongruous
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.
Yeah I was wondering the same thing. I then thought that separating this out into a different build would complicate the whole build for no good practical reason, just aesthetics.