[Sim] Replace sim.func.dpi.call with sim.func.call and improve a verifier, NFC #7452
Conversation
uenoku
requested review from
darthscsi,
seldridge,
fabianschuiki and
maerhart
as code owners
…fier Just drop `.dpi` Since sim.func.dpi.call now accepts func.func as well.
uenoku
force-pushed
the
dev/hidetou/sim-dpi-call
branch
from
25f72a1
to
4b62e79
Compare
There was a problem hiding this comment.
From a more general perspective I'm wondering if the motivation to let this call operation call func.func operations was only to get it lowered through Arc or whether there are other use-cases that will require this? Because there we could also lower it to func.call directly with a bit more work and thus simplifying the sim.func.call operation again (e.g., by placing a func.call inside an arc.clock_domain once it's fully supported). There'd probably be some value in keeping DPI functions and calls, and regular functions and calls separate (e.g., to keep the IR simple and elegant, and to not mix abstraction levels). But I'm also interested in what other people think about this.
The improved verifier is great regardless 🎉
| let description = [{ | ||
| `sim.func.dpi.call` represents SystemVerilog DPI function call. There are two | ||
| optional operands `clock` and `enable`. | ||
| `sim.func.call` represents a function call. Unlike a call operation in Func dialect, |
There was a problem hiding this comment.
| `sim.func.call` represents a function call. Unlike a call operation in Func dialect, | |
| `sim.func.call` represents a function call. Unlike a call operation in the Func dialect, |
| `sim.func.dpi.call` represents SystemVerilog DPI function call. There are two | ||
| optional operands `clock` and `enable`. | ||
| `sim.func.call` represents a function call. Unlike a call operation in Func dialect, | ||
| there are two optional operands `clock` and `enable`. | ||
|
|
||
| If `clock` is not provided, the callee is invoked when input values are changed. |
There was a problem hiding this comment.
| If `clock` is not provided, the callee is invoked when input values are changed. | |
| If `clock` is not provided, the callee is invoked when any call argument value changes. |
| `sim.func.dpi.call` represents SystemVerilog DPI function call. There are two | ||
| optional operands `clock` and `enable`. | ||
| `sim.func.call` represents a function call. Unlike a call operation in Func dialect, | ||
| there are two optional operands `clock` and `enable`. | ||
|
|
||
| If `clock` is not provided, the callee is invoked when input values are changed. | ||
| If provided, the DPI function is called at clock's posedge. The result values behave |
There was a problem hiding this comment.
| If provided, the DPI function is called at clock's posedge. The result values behave | |
| If provided, the DPI function is called at the clock's posedge. The result values behave |
| `enable` operand is used to conditionally call the DPI since DPI call could be quite | ||
| more expensive than native constructs. When `enable` is low, results of unclocked | ||
| calls are undefined and in SV results they are lowered into `X`. Users are expected | ||
| `enable` operand is used to conditionally call the function since function call |
There was a problem hiding this comment.
| `enable` operand is used to conditionally call the function since function call | |
| The `enable` operand is used to conditionally call the function since the call |
| calls are undefined and in SV results they are lowered into `X`. Users are expected | ||
| `enable` operand is used to conditionally call the function since function call | ||
| could be quite more expensive than native constructs. When `enable` is low, results | ||
| of unclocked calls are undefined and in SV they are lowered into `X`. Users are expected |
There was a problem hiding this comment.
I assume the same kind of 'undefined' as division by zero and out-of-bounds array accesses?
There was a problem hiding this comment.
Yes, this belong to the same kind to div-by-zero. I intended undefined values definition in comb https://circt.llvm.org/docs/Dialects/Comb/RationaleComb/#undefined-values. I'll mention it in here.
| calls are undefined and in SV results they are lowered into `X`. Users are expected | ||
| `enable` operand is used to conditionally call the function since function call | ||
| could be quite more expensive than native constructs. When `enable` is low, results | ||
| of unclocked calls are undefined and in SV they are lowered into `X`. Users are expected |
There was a problem hiding this comment.
I'm not really convinced that specifying how something is lowered to SV is a good way to specify an operation's semantics. Is it possible to define the semantics independent of SV in a way that allows this as a potential lowering?
There was a problem hiding this comment.
That's great point, thank you for suggestion!
| hw.module @dpi_call(in %clock : !seq.clock, in %in: i1) { | ||
| // expected-error @below {{'sim.func.call' op result type mismatch at index 1}} | ||
| // expected-note @below {{op result types: 'i1', 'i1'}} | ||
| %0, %1 = sim.func.call @mismatch(%in) : (i1) -> (i1, i1) |
There was a problem hiding this comment.
The "incorrect number of ..." error messages are not tested here.
Drop
.dpiofsim.func.dpi.callsince it now acceptsfunc.funcas well. This PR also adds nicer verifier for sim.func.call to check function types.