Add DPI intrinsics and API #4158

uenoku · 2024-06-10T07:10:13Z

This PR adds DPI intrinsic support and API to define DPI functions declaratively. The test passes locally but requires a new version of firtool.

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Feature (or new API)

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

uenoku · 2024-06-12T14:32:09Z

src/test/scala/chiselTests/DPISpec.scala

+                   |    std::cout << "hello from c++\\n";
+                   |}
+                   |
+                   |extern "C" void add(int lhs, int rhs, int* result)


comment for link.

sequencer

just small thoughts.

sequencer · 2024-06-13T03:40:57Z

src/main/scala/chisel3/util/circt/DPI.scala

+    * val a = RawClockedNonVoidFunctionCall("dpi_func_foo", UInt(1.W), clock, enable, b, c)
+    * }}}
+    */
+  def apply[T <: Data](functionName: String, ret: => T)(clock: Clock, enable: Bool, data: Data*): Data = {


what if the return type being a aggregate?

You can return passive aggregate. I'll add a similar document but here is ABI for DPI: llvm/circt#7163

sequencer · 2024-06-13T03:44:48Z

src/test/scala/chiselTests/DPISpec.scala

+  val result = RawClockedNonVoidFunctionCall("add", UInt(32.W))(clock, true.B, io.a, io.b)
+
+  // A function that allocates a state and returns a pointer.
+  val counter = RawClockedNonVoidFunctionCall("create_counter", UInt(64.W))(clock, reset.asBool)


Originally in SystemVerilog, if put multiple RawClockedNonVoidFunctionCall in an always block, they will be executed in order. This is be used to schedule the order of different DPI calls with dependencies, e.g. peek valid signal and poke the ready signal for handshake. It seems that we need to merge two DPIs together to achieve this?

It seems that we need to merge two DPIs together to achieve this?

Exactly, that's recommended way to model DPI dependency. It's extremely hard and breaking change to represent dependency within always clock with the current FIRRTL representation.

sequencer · 2024-06-13T03:52:03Z

src/main/scala/chisel3/util/circt/DPI.scala

+    * val a = RawUnlockedNonVoidFunctionCall("dpi_func_foo", UInt(1.W), enable, b, c)
+    * }}}
+    */
+  def apply[T <: Data](functionName: String, ret: => T)(enable: Bool, data: Data*): Data = {


I have been thinking: how to pass a string to DPI in Chisel, as well as Plusarg, maybe we also need a StringLit type in Chisel.

Yes. We'd like to pass and create a string, open array or chandle as well (chandle can be representable with UInt<64> though). My plan is to introduce "intrinsic/foreign type" concept so they can be created by intrinsics and that's orthogonal proposal to DPI.

I think PlusArg already have intrinsic so I guess we can use that?

But we may need a Chisel String data type.

Yes, I think creating Chisel/FIRRTL String type is a right way but it will require more design discussion and lots of implementation. I guess we can use Vec<UInt<8>, n> as a way to represent string for now :)

mind blowing. But yes, we can do it

src/main/scala/chisel3/util/circt/DPI.scala

sequencer · 2024-06-19T18:43:15Z

Maybe RawUnclockedVoidFunctionCall is missing?

sequencer · 2024-06-19T18:48:42Z

Two questions:

How to add a DPI function which is called at initial?(maybe need some preset flag to true.B as guard of this DPI, then configured it to false after initial)
How to export DPI functions? This is not designed for driver, but can be used for logging.

uenoku · 2024-06-19T19:51:22Z

Maybe RawUnclockedVoidFunctionCall is missing?

It's intentional since it's illegal to call void-function on non-procedural region.

How to add a DPI function which is called at initial?(maybe need some preset flag to true.B as guard of this DPI, then configured it to false after initial)

Good point. This is not currently supported. The reason why we cannot simply add sort of initial flag to DPI call op is that it will create dependency between two values in the same timing, e.g:

wire w;
node b = dpi_call_initial<func>(w);
node a = dpi_call_initial<func>(b);
w <= a

--> 

initial begin
   a = func(b);
   b = func(a);
end

or 

initial begin
   b = func(a);
   a = func(b);
end

I think we can restrict operands of dpi_call_initial op to be constant types but there are few other representation so I'm still iterating on the IR design.

How to export DPI functions? This is not designed for driver, but can be used for logging.

This is out-of-scope now since currently FIRRTL has nothing to export :) I think we first need to introduce FIRRTL functions as first class citizen. CIRCT SV dialect recently got function support but for FIRRTL side it's still in design phase .

sequencer · 2024-06-21T09:57:17Z

Another nit pick: when passing Reg to DPI, it will trigger the comb logic cycle error.

sequencer

Some nit on the return tpe

src/main/scala/chisel3/util/circt/DPI.scala

uenoku · 2024-07-05T07:40:25Z

Sorry for delay. It's ready for review now.

uenoku · 2024-07-08T08:54:09Z

src/test/scala/chiselTests/DPISpec.scala

+  Hello()
+  val result_clocked = AddClocked(io.a, io.b)
+  val result_unclocked = AddUnclocked(io.a, io.b)


comment for link

fabianschuiki

Very exciting to see this work!

src/test/scala/chiselTests/DPISpec.scala

jackkoenig · 2024-07-09T18:30:17Z

src/test/scala/chiselTests/DPISpec.scala

+object Hello extends DPIClockedVoidFunctionImport {
+  val functionName = "hello"
+  final def apply() = super.call()
+}
+
+object AddClocked extends DPINonVoidFunctionImport[UInt] {
+  override val functionName = "add"
+  val ret = UInt(32.W)


I'm of two minds of this API here. It's common in Chisel for user APIs to be inheritance based, but I don't think it's generally the best option. I'm not sure if I have a better idea though and I don't want to sandbag so maybe we just go with it and change it later if we need to. Any thoughts @seldridge?

There's some precedence for this kind of API for things like layers where you want to "declare" something and then refer to it multiple times. Using the object here achieves that (though I mentioned to @uenoku to self-type this with this: Singleton => to mandate an object). Also, this could be slightly cleaner as an abstract class in order to avoid the copious overrides.

Let's try this and see where it bottoms out.

Sounds good to me, thanks @seldridge!

src/main/scala/chisel3/util/circt/DPI.scala

jackkoenig

This is fantastic, thank you @uenoku!

jackkoenig · 2024-07-11T15:28:02Z

src/test/scala/chiselTests/DPISpec.scala

+        }
+        .result
+
+      val outputFile = io.Source.fromFile("test_run_dir/simulator/DPIIntrinsic/workdir-verilator/simulation-log.txt")


Ideally we would be a little DRY-er on these paths, deriving them from values defined above, but I'm not gonna block this PR on that.

dtzSiFive · 2024-07-11T15:58:41Z

Great work! 🚀

uenoku · 2024-07-11T17:12:12Z

Thank you for review!

sequencer · 2024-07-18T17:38:36Z

For those who come into this PR, and interested in the usage of Chisel DPI, here is a concrete opensource example of using DPIIntrinsic + verilator:

all signals are driven by DPI, see:
https://github.com/chipsalliance/t1/blob/master/ipemu/src/TestBench.scala
https://github.com/chipsalliance/t1/blob/master/ipemu/src/AXI4Agent.scala
compile verilog via verialtor and it will elaborate header files, see:
https://github.com/chipsalliance/t1/blob/master/difftest/online_drive/dpi_c/dpi.h
implement them in the native languages(C++, C, Rust), (T1 use rust):
https://github.com/chipsalliance/t1/blob/master/difftest/online_drive/src/dpi.rs

then link the dpi w/ verilator, then you can drive the Verilog with your native language then.

VCS should work as well, but due to the NDA limitation those cannot be opensourced.

Add a raw API for DPI intrinsics

c15f22d

uenoku added the Feature New feature, will be included in release notes label Jun 10, 2024

uenoku requested review from jackkoenig and debs-sifive June 10, 2024 07:10

Use C-types

7ce8e5b

uenoku commented Jun 12, 2024

View reviewed changes

sequencer reviewed Jun 13, 2024

View reviewed changes

sequencer reviewed Jun 19, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

sequencer reviewed Jun 22, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

sequencer mentioned this pull request Jun 23, 2024

ChiselSim tests runs orders of magnitude slower than chiseltest #4207

Open

Add comments. Add input and output names field

5b9301f

uenoku changed the base branch from main to ci/ci-circt-nightly July 5, 2024 07:39

uenoku marked this pull request as ready for review July 5, 2024 07:39

uenoku changed the title ~~Add a raw API for DPI intrinsics~~ Add DPI intrinsics and API Jul 8, 2024

uenoku force-pushed the dev/hidetou/dpi branch from f065389 to cf4b0d1 Compare July 8, 2024 08:50

uenoku commented Jul 8, 2024

View reviewed changes

Add DPI API

af89a23

uenoku force-pushed the dev/hidetou/dpi branch from cf4b0d1 to af89a23 Compare July 8, 2024 11:21

jackkoenig added this to the 7.0 milestone Jul 8, 2024

github-actions bot force-pushed the ci/ci-circt-nightly branch from 1f3caf4 to cc782e9 Compare July 9, 2024 11:05

fabianschuiki approved these changes Jul 9, 2024

View reviewed changes

src/test/scala/chiselTests/DPISpec.scala Outdated Show resolved Hide resolved

jackkoenig force-pushed the ci/ci-circt-nightly branch from cc782e9 to 2a8affd Compare July 9, 2024 17:41

jackkoenig reviewed Jul 9, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jul 9, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jul 9, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Show resolved Hide resolved

sequencer reviewed Jul 9, 2024

View reviewed changes

src/main/scala/chisel3/util/circt/DPI.scala Show resolved Hide resolved

Add final/override

0ca6c88

github-actions bot force-pushed the ci/ci-circt-nightly branch 2 times, most recently from ac00864 to 4a00443 Compare July 11, 2024 11:05

jackkoenig approved these changes Jul 11, 2024

View reviewed changes

jackkoenig merged commit bba4610 into chipsalliance:ci/ci-circt-nightly Jul 11, 2024
13 checks passed

github-actions bot pushed a commit that referenced this pull request Jul 12, 2024

Add DPI intrinsics and API (#4158)

7a8c16d

chiselbot pushed a commit that referenced this pull request Jul 13, 2024

Add DPI intrinsics and API (#4158)

755373c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DPI intrinsics and API #4158

Add DPI intrinsics and API #4158

uenoku commented Jun 10, 2024 •

edited by jackkoenig

Loading

uenoku Jun 12, 2024

sequencer left a comment

sequencer Jun 13, 2024

uenoku Jun 13, 2024

sequencer Jun 13, 2024

uenoku Jun 13, 2024

sequencer Jun 13, 2024

uenoku Jun 13, 2024 •

edited

Loading

uenoku Jun 13, 2024

sequencer Jun 17, 2024

uenoku Jun 17, 2024 •

edited

Loading

sequencer Jun 17, 2024

sequencer commented Jun 19, 2024

sequencer commented Jun 19, 2024

uenoku commented Jun 19, 2024

sequencer commented Jun 21, 2024

sequencer left a comment

uenoku commented Jul 5, 2024

uenoku Jul 8, 2024

fabianschuiki left a comment

jackkoenig Jul 9, 2024

seldridge Jul 11, 2024

jackkoenig Jul 11, 2024

jackkoenig left a comment

jackkoenig Jul 11, 2024

dtzSiFive commented Jul 11, 2024

uenoku commented Jul 11, 2024

sequencer commented Jul 18, 2024 •

edited

Loading

Add DPI intrinsics and API #4158

Add DPI intrinsics and API #4158

Conversation

uenoku commented Jun 10, 2024 • edited by jackkoenig Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Choose a reason for hiding this comment

sequencer 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

uenoku Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sequencer commented Jun 19, 2024

sequencer commented Jun 19, 2024

uenoku commented Jun 19, 2024

sequencer commented Jun 21, 2024

sequencer left a comment

Choose a reason for hiding this comment

uenoku commented Jul 5, 2024

Choose a reason for hiding this comment

fabianschuiki 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

jackkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive commented Jul 11, 2024

uenoku commented Jul 11, 2024

sequencer commented Jul 18, 2024 • edited Loading

uenoku commented Jun 10, 2024 •

edited by jackkoenig

Loading

uenoku Jun 13, 2024 •

edited

Loading

uenoku Jun 17, 2024 •

edited

Loading

sequencer commented Jul 18, 2024 •

edited

Loading