-
Notifications
You must be signed in to change notification settings - Fork 76
Fix issue #428: add blackbox sources to argument of icarus-verilog and vcs #429
Conversation
5d2cbd9
to
673c2d5
Compare
It seems that the vcs executable is ran under the project root directory. Update: FIXED now. |
673c2d5
to
b67172b
Compare
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.
Thanks for improving the VCS backend. I am looking forward to your responses.
@@ -7,6 +7,7 @@ import firrtl.options.Dependency | |||
import firrtl.stage.RunFirrtlTransformAnnotation | |||
import firrtl.transforms.formal.RemoveVerificationStatements | |||
import firrtl.{AnnotationSeq, CircuitState} | |||
import scala.io.Source |
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.
What is this import needed for?
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.
Removed.
@@ -120,7 +121,8 @@ private object IcarusSimulator extends Simulator { | |||
val simBinary = relTargetDir / "icarus" / topName | |||
val cmd = List("iverilog") ++ flags ++ List( | |||
"-o", | |||
simBinary.toString(), | |||
simBinary.toString() | |||
) ++ BlackBox.blackBoxSources(targetDir) ++ List( |
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.
Icarus supports .f
files with the -f
option. I would prefer if you could use that to keep things more uniform to how the Verilator backend works.
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.
Done.
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.
However, it is not working. Let me investigate.
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.
Sadly, the -f
option of icarus does not work as expected. The problem is that iverilog expect .f
to end with endline. In iverilog 11.0, it silently fails and ignores the file.
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.
oh wow, that sucks
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 submitted a PR to firrtl: chipsalliance/firrtl#2405
@@ -111,7 +111,8 @@ private object VcsSimulator extends Simulator { | |||
annos: AnnotationSeq | |||
): Seq[String] = { | |||
val flags = generateFlags(topName, targetDir, annos) | |||
val cmd = List("vcs") ++ flags ++ List("-o", topName, s"$topName.sv", verilogHarness, "vpi.cpp") | |||
val cmd = List("vcs") ++ flags ++ List("-o", topName) ++ | |||
BlackBox.blackBoxSources(targetDir) ++ List(s"$topName.sv", verilogHarness, "vpi.cpp") |
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.
Does VCS support .f
files with the -f
flag? If so, please use that approach to stay as close as possible to what we do in the Verilator backend.
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.
Yes:
-f filename
Specify a file that contains a list of source files and compile-time
options, including C source files and object files.
} | ||
|
||
/** finds paths to blackbox sources */ | ||
private[chiseltest] def blackBoxSources(targetDir: os.Path): Seq[String] = { |
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.
Ideally this function will not be needed (see comments below)
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.
Removed.
|
||
import scala.io.Source | ||
|
||
object BlackBox { |
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.
please mark this object private
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.
Done.
@@ -90,7 +90,7 @@ private object VcsSimulator extends Simulator { | |||
waveformFlags(toplevel.name, state.annotations) | |||
|
|||
// the binary we created communicates using our standard IPC interface | |||
new IPCSimulatorContext(simCmd, toplevel, VcsSimulator) | |||
new IPCSimulatorContext(simCmd, targetDir, toplevel, VcsSimulator) |
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.
Could you elaborate on why you need VCS to run in the targetDir
instead of the os.pwd
? The problem here is that the Verilator simulation will always run in os.pwd
and thus I would prefer to keep that the same for all simulators to ensure uniform behavior.
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.
VCS writes vcd file and ucli.key
to cwd, and these files should be put under targetDir.
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.
In the current master, VCS backend fails with ./topLevel
executable not found.
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 think there might be a different solution to this problem. Would you mind putting your second commit in a separate PR? It would be convenient if we could review the two issues separately.
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.
Okay.
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.
See pr #430.
To elaborate on the issue of where the simulator is run (i.e. what working directory it uses):
|
c0a49d6
to
a081428
Compare
a081428
to
d3b9854
Compare
Regarding the icarus issue, possible fixes are:
|
Thanks for debugging. I will try address this upstream. |
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.
Let's get this in and then fix the issue in upstream firrtl.
No description provided.