Enhance test and document for DPI and tweak trait name. #4380

uenoku · 2024-09-04T16:41:28Z

This PR adds a test for open array and document docs/src/explanations/dpi.md which describes DPI API usage.
This also changes DPIClockedVoidFunctionImport to DPIVoidFunctionImport for more consistent naming (this is breaking change but I believe there is no user of DPIClockedVoidFunctionImport so it should be ok).

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

API modification
Documentation or website-related

Desired Merge Strategy

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

Release Notes

Enhance test and document for DPI and tweak trait name

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.

docs/src/explanations/dpi.md

mwachs5 · 2024-09-04T17:13:27Z

docs/src/explanations/dpi.md

+  final def apply() = super.call()
+}
+
+Hello() // Print


should this Hello() call be inside a Module?

Yes. The snippet was meant to be pseudo code so I'll update with mdoc.

mwachs5 · 2024-09-04T17:13:43Z

docs/src/explanations/dpi.md

+
+To call this function from Chisel, we need to define a corresponding DPI object.
+
+```scala


can you use mdoc so that we can check the code compiles at least?

Yes, thank you for pointing out. Totally missed there is mdoc support in chisel repo as well.

mwachs5 · 2024-09-04T17:15:05Z

docs/src/explanations/dpi.md

+* A vector is lowered to an *unpacked* *open* array type, e.g., `a: Vec<4, UInt>` to `byte a []`.
+* A bundle is lowered to a packed struct.


Suggested change

* A vector is lowered to an *unpacked* *open* array type, e.g., `a: Vec<4, UInt>` to `byte a []`.

* A bundle is lowered to a packed struct.

* A `Vec` is lowered to an *unpacked* *open* array type, e.g., `a: Vec<4, UInt>` to `byte a []`.

* A `Bundle` is lowered to a packed struct.

for vec i think you mean a chisel3.Vec not a scala Vector, right?

Yes, chisel3.Vec.

mwachs5 · 2024-09-04T17:15:53Z

docs/src/explanations/dpi.md

+* Operand and result types must be passive.
+* A vector is lowered to an *unpacked* *open* array type, e.g., `a: Vec<4, UInt>` to `byte a []`.
+* A bundle is lowered to a packed struct.
+* Integer types are lowered into 2-state types.


Suggested change

* Integer types are lowered into 2-state types.

* `Int`, `SInt`, `Clock`, `Reset` types are lowered into 2-state types.

? Are clock and reset lowered?

I am not actually sure if you are talkign about Chisel Data subclasses or not

mwachs5 · 2024-09-04T17:17:30Z

docs/src/explanations/dpi.md

+  final def apply(lhs: UInt, rhs: UInt): UInt = super.call(lhs, rhs)
+}
+
+val result = Add(2.U(32.W), 3.U(32.W))


again would be helpful to see this with mdoc, I assume this Add function belongs inside a Module or a RawModule given that we say clocked is False

mwachs5 · 2024-09-04T17:17:48Z

docs/src/explanations/dpi.md

+
+* `Add` inherits from `DPINonVoidFunctionImport[UInt]` because it returns a 32-bit unsigned integer.
+* `ret` specifies the return type.
+* `clocked` indicates that this is a clocked function call.


but this is set to false so isn't NOT a clocked function call?

mwachs5 · 2024-09-04T17:19:15Z

docs/src/explanations/dpi.md

+object Sum extends DPINonVoidFunctionImport[UInt] {
+  override val functionName = "sum"
+  override val ret = UInt(32.W)
+  override val clocked = false


should clocked be true?

For OpenArray example both clocked and unclocked work. So I didn't explicitly describe the example. It depends on whether user wants to evaluate the value at every input value changes or clock's posedge.
This is an example for Add

chisel/src/test/scala/chiselTests/DPISpec.scala

Lines 158 to 161 in ead5fc9

dpi.io.add_clocked_result.peek()

dpi.io.add_clocked_result.expect(60)

dpi.io.add_unclocked_result.peek()

dpi.io.add_unclocked_result.expect(36)

mwachs5 · 2024-09-04T17:19:34Z

docs/src/explanations/dpi.md

+}
+```
+
+```scala


again please use mdoc to check for compile if not chisel elaboration

mwachs5 · 2024-09-04T17:20:39Z

docs/src/explanations/dpi.md

+```
+
+# FAQ
+


can you show an example where having clocked = false makes sense? Is that done in a when context...?

Unclocked calls be useful when user wants to replace pure (but expensive) combinatorial logics with dpi call (actually Add is a good example of such use case).
An unclocked call under when(cond) is lowered into always_comb + if(cond) and DPI is conditionally invoked as well. I updated Add example to include both of them.

class AddTest extends Module { val io = IO(new Bundle { val a = Input(UInt(32.W)) val b = Input(UInt(32.W)) val c = Output(UInt(32.W)) val d = Output(UInt(32.W)) val en = Input(Bool()) }) // Call DPI only when `en` is true. when (io.en) { io.c := AddClocked(io.a, io.b) io.d := AddUnclocked(io.a, io.b) } .otherwise { io.c := 0.U(32.W) io.d := 0.U(32.W) } }

module AddTest( input clock, reset, input [31:0] io_a, io_b, output [31:0] io_c, io_d, input io_en ); logic [31:0] _add_0; reg [31:0] _GEN; always @(posedge clock) begin if (io_en) begin add(io_a, io_b, _add_0); _GEN <= _add_0; end end // always @(posedge) reg [31:0] _GEN_0; always_comb begin if (io_en) begin add(io_a, io_b, _GEN_0); end else _GEN_0 = 32'bx; end // always_comb assign io_c = io_en ? _GEN : 32'h0; assign io_d = io_en ? _GEN_0 : 32'h0;

mmaloney-sf · 2024-09-04T19:54:32Z

I'm not 100% sure of Chisel's documentation conventions, but quickly glancing at other .md files, I think we should stick to a "one sentence per line" convention.

This makes diffs easier to read.

docs/src/explanations/dpi.md

mmaloney-sf · 2024-09-04T20:02:17Z

docs/src/explanations/dpi.md

+
+## Type ABI
+
+Unlike normal Chisel compilation flow, we use a specific ABI for types to interact with DPI.


I'm not sure what this means. Could you elaborate?

It may also be useful to explicitly point out that what we're doing in this section is showing how Chisel types map to C types.

mmaloney-sf · 2024-09-04T20:12:11Z

docs/src/explanations/dpi.md

+Here's an example of a DPI function that calculates the sum of two numbers:
+
+```c++
+extern "C" void add(int lhs, int rhs, int* result)


It surprised me to see here that the int* result is an out-parameter. Is that always the case? (If so, maybe it needs to be called out earlier. Or maybe I just missed it?)

Thanks, I thought I clarified that but apparently it was dropped somewhere. I'll add explanation for that.

mmaloney-sf · 2024-09-04T20:14:22Z

docs/src/explanations/dpi.md

+Chisel vectors are converted into SystemVerilog open arrays when used with DPI. Since memory layout can vary between simulators, it's recommended to use `svSize` and `svGetBitArrElemVecVal` to access array elements.
+
+```c++
+extern "C" void sum(const svOpenArrayHandle array, int* result) {


What is svOpenArrayHandle?

I guess, more generally, what libraries does Chisel depend on in the C++ DPI? We can't simply assume that a general Chisel user will have whatever library exposes DPI.h or whatever it is.

It would be good to have a section at the very start that gives the C++ prerequisites.

Thank you for the suggestion, that makese sense. SV Spec (Section 35) defineds svdpi.h which declares these functions and types (including svOpenArrayHandle). These are expected to be implemented by simulators. I'll create a section for that.

mmaloney-sf · 2024-09-04T20:15:11Z

docs/src/explanations/dpi.md

+
+# FAQ
+
+* Can we export functions? -- No, not currently. Consider using a black box for such functionality.


What do you mean by "export" here?

mmaloney-sf · 2024-09-04T20:17:29Z

docs/src/explanations/dpi.md

+
+* Can we export functions? -- No, not currently. Consider using a black box for such functionality.
+* Can we call a DPI function in initial block? -- No, not currently. Consider using a black box for initialization.
+* Can we call two clocked DPI calls and pass the result to another within the same clock? -- No, not currently. Please merge the DPI functions into a single function.


I'm not sure what this last point means.

It feels like what we actually want is a light explanation of the underlying "framework" for the DPI calls, and that the above comment is actually a restriction imposed on this framework.

For example, is this true?: "All DPI calls on the same clock are dispatched at the same time. Because of this, it is not possible to use the result of one DPI call as an argument to another."

All DPI calls on the same clock are dispatched at the same time. Because of this, it is not possible to use the result of one DPI call as an argument to another

Yes, that's correct. This is fundamental restrictions for side-effecting operations in Chisel but DPI is a first example that has both side-effect and results.

mmaloney-sf · 2024-09-04T20:19:04Z

src/test/scala/chiselTests/DPISpec.scala

@@ -17,6 +17,7 @@ private object EmitDPIImplementation {
    val dpiImpl = s"""
                     |#include <stdint.h>
                     |#include <iostream>
+                     |#include <svdpi.h>


To reiterate an earlier point: Let's add a prerequisites section near the top. Let's make sure to name what library(ies?) are needed for Chisel DPI, and let's be sure to include the svdpi.h header in our examples, since otherwise, the code doesn't work.

Let's also link to the documentation so that Chisel users can quickly cross-reference the DPI primitives, such as svBitVecVal or svOpenArrayHandle.

uenoku

Thank you for comments! I added code examples with mdoc. I'll mark it draft until all comments were addressed.

uenoku · 2024-09-05T11:48:54Z

docs/src/explanations/dpi.md

+* A vector is lowered to an *unpacked* *open* array type, e.g., `a: Vec<4, UInt>` to `byte a []`.
+* A bundle is lowered to a packed struct.


Yes, chisel3.Vec.

uenoku · 2024-09-05T11:57:28Z

docs/src/explanations/dpi.md

+Here's an example of a DPI function that calculates the sum of two numbers:
+
+```c++
+extern "C" void add(int lhs, int rhs, int* result)


Thanks, I thought I clarified that but apparently it was dropped somewhere. I'll add explanation for that.

uenoku · 2024-09-05T12:06:51Z

docs/src/explanations/dpi.md

+Chisel vectors are converted into SystemVerilog open arrays when used with DPI. Since memory layout can vary between simulators, it's recommended to use `svSize` and `svGetBitArrElemVecVal` to access array elements.
+
+```c++
+extern "C" void sum(const svOpenArrayHandle array, int* result) {


Thank you for the suggestion, that makese sense. SV Spec (Section 35) defineds svdpi.h which declares these functions and types (including svOpenArrayHandle). These are expected to be implemented by simulators. I'll create a section for that.

uenoku · 2024-09-05T13:22:04Z

docs/src/explanations/dpi.md

+object Sum extends DPINonVoidFunctionImport[UInt] {
+  override val functionName = "sum"
+  override val ret = UInt(32.W)
+  override val clocked = false


For OpenArray example both clocked and unclocked work. So I didn't explicitly describe the example. It depends on whether user wants to evaluate the value at every input value changes or clock's posedge.
This is an example for Add

chisel/src/test/scala/chiselTests/DPISpec.scala

Lines 158 to 161 in ead5fc9

dpi.io.add_clocked_result.peek()

dpi.io.add_clocked_result.expect(60)

dpi.io.add_unclocked_result.peek()

dpi.io.add_unclocked_result.expect(36)

uenoku · 2024-09-05T13:29:51Z

docs/src/explanations/dpi.md

+
+To call this function from Chisel, we need to define a corresponding DPI object.
+
+```scala


Yes, thank you for pointing out. Totally missed there is mdoc support in chisel repo as well.

uenoku · 2024-09-05T13:30:38Z

docs/src/explanations/dpi.md

+  final def apply() = super.call()
+}
+
+Hello() // Print


Yes. The snippet was meant to be pseudo code so I'll update with mdoc.

uenoku · 2024-09-05T13:37:07Z

docs/src/explanations/dpi.md

+
+* Can we export functions? -- No, not currently. Consider using a black box for such functionality.
+* Can we call a DPI function in initial block? -- No, not currently. Consider using a black box for initialization.
+* Can we call two clocked DPI calls and pass the result to another within the same clock? -- No, not currently. Please merge the DPI functions into a single function.


All DPI calls on the same clock are dispatched at the same time. Because of this, it is not possible to use the result of one DPI call as an argument to another

Yes, that's correct. This is fundamental restrictions for side-effecting operations in Chisel but DPI is a first example that has both side-effect and results.

uenoku · 2024-09-05T13:54:12Z

docs/src/explanations/dpi.md

+```
+
+# FAQ
+


Unclocked calls be useful when user wants to replace pure (but expensive) combinatorial logics with dpi call (actually Add is a good example of such use case).
An unclocked call under when(cond) is lowered into always_comb + if(cond) and DPI is conditionally invoked as well. I updated Add example to include both of them.

class AddTest extends Module { val io = IO(new Bundle { val a = Input(UInt(32.W)) val b = Input(UInt(32.W)) val c = Output(UInt(32.W)) val d = Output(UInt(32.W)) val en = Input(Bool()) }) // Call DPI only when `en` is true. when (io.en) { io.c := AddClocked(io.a, io.b) io.d := AddUnclocked(io.a, io.b) } .otherwise { io.c := 0.U(32.W) io.d := 0.U(32.W) } }

module AddTest( input clock, reset, input [31:0] io_a, io_b, output [31:0] io_c, io_d, input io_en ); logic [31:0] _add_0; reg [31:0] _GEN; always @(posedge clock) begin if (io_en) begin add(io_a, io_b, _add_0); _GEN <= _add_0; end end // always @(posedge) reg [31:0] _GEN_0; always_comb begin if (io_en) begin add(io_a, io_b, _GEN_0); end else _GEN_0 = 32'bx; end // always_comb assign io_c = io_en ? _GEN : 32'h0; assign io_d = io_en ? _GEN_0 : 32'h0;

uenoku added the Documentation Only changing documentation label Sep 4, 2024

Add more document for DPI and tweak trait name.

5a1799e

uenoku force-pushed the dev/hidetou/dpi-document-2 branch from 7b82375 to 5a1799e Compare September 4, 2024 16:44

uenoku requested a review from jackkoenig September 4, 2024 16:49

mwachs5 reviewed Sep 4, 2024

View reviewed changes

docs/src/explanations/dpi.md Show resolved Hide resolved

mwachs5 reviewed Sep 4, 2024

View reviewed changes

docs/src/explanations/dpi.md Outdated

}

```

```scala

Copy link

Contributor

mwachs5 Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again please use mdoc to check for compile if not chisel elaboration

mwachs5 reviewed Sep 4, 2024

View reviewed changes

mmaloney-sf suggested changes Sep 4, 2024

View reviewed changes

Address some comments(not finished all yet) and code examples

d6128dc

uenoku commented Sep 5, 2024

View reviewed changes

uenoku marked this pull request as draft September 5, 2024 13:58


		To call this function from Chisel, we need to define a corresponding DPI object.

		```scala

		* A vector is lowered to an unpacked open array type, e.g., `a: Vec<4, UInt>` to `byte a []`.
		* A bundle is lowered to a packed struct.

	* Integer types are lowered into 2-state types.
	* `Int`, `SInt`, `Clock`, `Reset` types are lowered into 2-state types.

	dpi.io.add_clocked_result.peek()
	dpi.io.add_clocked_result.expect(60)
	dpi.io.add_unclocked_result.peek()
	dpi.io.add_unclocked_result.expect(36)


		## Type ABI

		Unlike normal Chisel compilation flow, we use a specific ABI for types to interact with DPI.


		# FAQ

		* Can we export functions? -- No, not currently. Consider using a black box for such functionality.

Enhance test and document for DPI and tweak trait name. #4380

Are you sure you want to change the base?

Enhance test and document for DPI and tweak trait name. #4380

Conversation

uenoku commented Sep 4, 2024

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmaloney-sf commented Sep 4, 2024

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

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 left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku left a comment •

edited

Loading