[Sim][SimToSv] DPI-C Semantic preserving sim.func.dpi and sim.func.dpi.call#9977
[Sim][SimToSv] DPI-C Semantic preserving sim.func.dpi and sim.func.dpi.call#9977
Conversation
uenoku
left a comment
There was a problem hiding this comment.
The proposal makes sense to me. Thank you for working on this! One high-level comment is can you consider introducing a type for dpi function that correctly capture sim.func.explicitly_returned and byref as more structured way instead of putting on operations? I admit I'm the one that abused ModuleType but would be fantastic if we can use this opportunity to clean up.
So, departing from the ModuleType is certainly an option. I considered it but it seemed a bit too invasive. I will try to do it. |
|
Thanks, that would be really great! |
71e2b6e to
c85d35e
Compare
Introduce DPIDirection enum (Input, Output, InOut, Return, Ref) and
DPIModuleType, a first-class MLIR type that bundles port name, type,
and direction. DPIFuncOp now stores DPIModuleType + FunctionType
instead of hw::ModuleType + per_argument_attrs.
- SimTypes: DPIPort struct, DPIModuleTypeStorage with cached index
arrays. Shared helpers (stringifyDPIDirectionKeyword,
parseDPIDirectionKeyword, isCallOperandDir) and
DPIModuleType::verify() for reuse by downstream dialects.
- SimOps: DPIFuncOp parser/printer with keyword syntax
(input/output/inout/return/ref). Verifier adds sim-specific
checks (ref→llvm.ptr, function_type consistency).
- LowerDPIFunc: Iterates DPIModuleType ports directly.
- SimToSV: Uses getHWModuleType() at conversion boundary.
- FIRRTL LowerDPI: Builds DPIModuleType; dedup keyed on
{StringAttr, DPIModuleType}.
- SimDPI.md: Elaborates design choices for the new semantics.
c85d35e to
269f033
Compare
OK. So it is done. Indeed more invasive but the semantics are much better |
@uenoku Hi, are you planning to review this PR? Is there anything else I should do? Is there anyone else I should contact somehow? I hope I am not being too bold, just noticed there wasn't any movement |
uenoku
left a comment
There was a problem hiding this comment.
Generally looks good to me. Sorry for delay.
| KeyTy getAsKey() const { return ports; } | ||
|
|
||
| llvm::ArrayRef<DPIPort> getPorts() const { return ports; } | ||
|
|
There was a problem hiding this comment.
Could you consider store mlir::FunctionType in the storage?
There was a problem hiding this comment.
DPIPort contains information about the direction and semantics of the arguments/ports. If we simply use FunctionType as storage we will not have this information which is integral to this change. Or maybe I misunderstood your meaning?
There was a problem hiding this comment.
Ah I meant to simply store mlir::FunctionType in DPIFunctionTypeStorage, instead of storing in DPIFuncOp since it a bit weird that DPIFuncOp takes two types
| auto it = state.dpiFuncDeclMapping.find(op.getCalleeAttr().getAttr()); | ||
| if (it != state.dpiFuncDeclMapping.end()) | ||
| op.setCallee(it->second.getSymNameAttr()); |
There was a problem hiding this comment.
What was the reason for this behavior change?
There was a problem hiding this comment.
My bad. There is no "real" need for this change, and it is actually a bad change as it is implemented now. I wanted to avoid an undefined behavior in case of malformed IR (which should actually be caught before) but ended up silently masking the error. I will emit an explicit failure
| sim.func.dpi @add_mlir(in %a : i32, in %b : i32, out c : i32) attributes {verilogName = "add_mlir_impl"} | ||
| sim.func.dpi @mul_shared(input %a : i32, input %b : i32, output c : i32) | ||
| sim.func.dpi @add_mlir(input %a : i32, input %b : i32, output c : i32) attributes {verilogName = "add_mlir_impl"} | ||
| func.func @add_mlir_impl(%arg0: i32, %arg1: i32, %arg2: !llvm.ptr) { |
There was a problem hiding this comment.
Not blocking, but does JIT work with inout already?
There was a problem hiding this comment.
As far as I can tell ARC goes through LowerDPIFunc so it should be supported. Should we add a test for it here? If it doesn't work, whould it be fixed as part of this PR?
There was a problem hiding this comment.
No I was just curios! Thank you for checking!
…i.call (llvm#9977) Introduce DPIDirection enum (Input, Output, InOut, Return, Ref) and DPIFunctionType, a first-class MLIR type that bundles port name, type, and direction. DPIFuncOp now stores DPIFunctionType + FunctionType instead of hw::ModuleType + per_argument_attrs. - SimTypes: DPIArgumentstruct, DPIFunctionTypeStorage with cached index arrays. Shared helpers (stringifyDPIDirectionKeyword, parseDPIDirectionKeyword, isCallOperandDir) and DPIModuleType::verify() for reuse by downstream dialects. - SimOps: DPIFuncOp parser/printer with keyword syntax (input/output/inout/return/ref). Verifier adds sim-specific checks (ref→llvm.ptr, function_type consistency). - LowerDPIFunc: Iterates DPIFunctionType ports directly. - FIRRTL LowerDPI: Builds DPIModuleType; dedup keyed on {StringAttr, DPIModuleType}. - SimDPI.md: Elaborates design choices for the new semantics. AI-Assisted-by: GitHub Copilot CLI with GPT-5.4 and Claude Opus 4.6
Introduce DPIDirection enum (Input, Output, InOut, Return, Ref) and
DPIFunctionType, a first-class MLIR type that bundles port name, type,
and direction. DPIFuncOp now stores DPIFunctionType + FunctionType
instead of hw::ModuleType + per_argument_attrs.
arrays. Shared helpers (stringifyDPIDirectionKeyword,
parseDPIDirectionKeyword, isCallOperandDir) and
DPIModuleType::verify() for reuse by downstream dialects.
(input/output/inout/return/ref). Verifier adds sim-specific
checks (ref→llvm.ptr, function_type consistency).
{StringAttr, DPIModuleType}.
AI-Assisted-by: GitHub Copilot CLI with GPT-5.4 and Claude Opus 4.6
This is the first of a 2 PR series introducing full sematic preserving DPI calls from SystemVerilog through Moore to Sim.