[Moore] Add module and instance port support #7112
Conversation
fabianschuiki
requested review from
uenoku,
maerhart,
dtzSiFive and
hailongSun2000
|
@cepheus69 @hailongSun2000 I collected the work you've been doing on module and instance ports and have combined all of that into a PR, with tests and a few tweaks. |
fabianschuiki
force-pushed
the
fschuiki/moore-hierarchy
branch
from
b5ebd1c
to
76a758e
Compare
|
Sounds great! 🎉 |
|
It is so great to see its landing. My implementations in my branch are not finished yet 🥲 which is not based on ModuleType. Because I am hesitant to utilize HW ModuleType, I designed a PortInfo Structure for the Moore dialect and added the corresponding builder, parser, verifier, and so on for SVModuleOp. I think I consume much time to struggle with the operation definition function. |
|
Cool! So elegently using these traits and can all writen in tablegen file. |
| // CHECK-SAME: a0: %x2: !moore.l1 | ||
| // CHECK-SAME: a1: %y2: !moore.l2 | ||
| // CHECK-SAME: ) -> ( | ||
| // CHECK-SAME: b0: !moore.i32 |
There was a problem hiding this comment.
I noticed that the print function of instanceOp has been changed, the output list of Moore InstanceOp moves away from the SSA value and changes to the name. We use assignment and the result of instanceOp to denote the connection. I think it's a reasonable change from designing an operation.
There was a problem hiding this comment.
My hope is that the module ports become extremely simple with this approach: they only copy SSA values into the module (instance operands, module in ports), or they copy SSA values out of the module (instance results, module out ports). All the strange Verilog features like net ports, variable ports, ref ports, multi ports, etc., are handled inside and outside the module as ops, but the ports themselves stay simple.
| // Prepare the values that are involved in port connections. This creates | ||
| // rvalues for input ports and appropriate lvalues for output, inout, and | ||
| // ref ports. We also separate multi-ports into the individual underlying | ||
| // ports with their corresponding connection. |
There was a problem hiding this comment.
From my perspective, in your case(module PortsAnsi), input--%a, inout--%c, and ref--%d are all regarded as the rhs of moore.assign. And only output--%d is the operand of moore.output.
There was a problem hiding this comment.
Yes absolutely! 🙂 My idea was to make almost everything an input and then use your cool new ref<T> type that you've been working on:
input TbecomesTinputinout Tbecomesref<T>input (maybe laternet<T>if we decide we need to distinguish vars and nets)ref Tbecomesref<T>inputoutput TbecomesToutput
At the instance, the input would be lowered as rvalue to get a T, and all others would be lowered as lvalue to get a ref<T>. The refs for inout and ref ports are passed directly to the instance as operands, and outputs generate a moore.assign with the lvalue on the left and the instance's output value on the right:
%a = moore.variable
%0 = moore.read %a // rvalue for `a`
%b = moore.variable // lvalue for `b`
%c = moore.net // lvalue for `c`
%d = moore.variable // lvalue for `d`
%1 = moore.instance ...(a: %0: T, c: %c: net<T>, d: %d: ref<T>) -> (b: T)
moore.assign %b, %1
|
I read the code in detail. I don't have any questions. 🎉 |
There was a problem hiding this comment.
Looks reasonable to me! Note that printModuleSignatureNew expects non-empty arrays for locations if mlir-print-debuginfo is provided so circt-translate --import-verilog basic.sv -o test.mlir -- mlir-print-debuginfo should cause assertion failure right now.
circt-translate: /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/ArrayRef.h:257: const T& llvm::ArrayRef<T>::operator[](size_t) const [with T = mlir::Location; size_t = long unsigned int]: Assertion `Index < Length && "Invalid index!"' failed.
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0. Program arguments: ./build/bin/circt-translate --import-verilog basic.sv -o test.mlir --mlir-print-debuginfo
#0 0x000062a9a9caf59f llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/uenoku/dev/circt/llvm/llvm/lib/Support/Unix/Signals.inc:727:3
#1 0x000062a9a9cacfff llvm::sys::RunSignalHandlers() /home/uenoku/dev/circt/llvm/llvm/lib/Support/Signals.cpp:105:20
#2 0x000062a9a9cad356 SignalHandler(int) /home/uenoku/dev/circt/llvm/llvm/lib/Support/Unix/Signals.inc:413:1
#3 0x000079699d242520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
#4 0x000079699d2969fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
#5 0x000079699d2969fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10
#6 0x000079699d2969fc pthread_kill ./nptl/pthread_kill.c:89:10
#7 0x000079699d242476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
#8 0x000079699d2287f3 abort ./stdlib/abort.c:81:7
#9 0x000079699d22871b _nl_load_domain ./intl/loadmsgcat.c:1177:9
#10 0x000079699d239e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#11 0x000062a9a9e674e2 decltype(auto) llvm::dyn_cast<mlir::DictionaryAttr, mlir::Attribute>(mlir::Attribute const&) /home/uenoku/dev/circt/llvm/llvm/include/llvm/Support/Casting.h:650:3
#12 0x000062a9a9e674e2 decltype(auto) llvm::dyn_cast<mlir::DictionaryAttr, mlir::Attribute>(mlir::Attribute const&) /home/uenoku/dev/circt/llvm/llvm/include/llvm/Support/Casting.h:649:37
#13 0x000062a9a9e674e2 circt::hw::module_like_impl::printModuleSignatureNew(mlir::OpAsmPrinter&, mlir::Region&, circt::hw::ModuleType, llvm::ArrayRef<mlir::Attribute>, llvm::ArrayRef<mlir::Location>) /home/uenoku/dev/circt/lib/Dialect/HW/ModuleImplementation.cpp:438:47
#14 0x000062a9a9eaecc2 circt::moore::SVModuleOp::print(mlir::OpAsmPrinter&) /home/uenoku/dev/circt/lib/Dialect/Moore/MooreOps.cpp:32:37
#15 0x000062a9a9e849aa void llvm::detail::UniqueFunctionBase<void, mlir::Operation*, mlir::OpAsmPrinter&, llvm::StringRef>::CallImpl<mlir::Op<circt::moore::SVModuleOp, mlir::OpTrait::OneRegion, mlir::OpTrait::ZeroResults, mlir::OpTrait::ZeroSuccessors, mlir::OpTrait::ZeroOperands, mlir::OpTrait::SingleBlock, mlir::OpTrait::SingleBlockImplicitTerminator<circt::moore::OutputOp>::Impl, mlir::OpTrait::OpInvariants, mlir::BytecodeOpInterface::Trait, mlir::OpTrait::IsIsolatedFromAbove, mlir::SymbolOpInterface::Trait, mlir::OpAsmOpInterface::Trait>::getPrintAssemblyFn()::'lambda'(mlir::Operation*, mlir::OpAsmPrinter&, llvm::StringRef) const>(void*, mlir::Operation*, mlir::OpAsmPrinter&, llvm::StringRef) /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/FunctionExtras.h:222:3
#16 0x000062a9a9ea1e11 llvm::unique_function<void (mlir::Operation*, mlir::OpAsmPrinter&, llvm::StringRef) const>::~unique_function() /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/FunctionExtras.h:390:7
#17 0x000062a9a9ea1e11 mlir::RegisteredOperationName::Model<circt::moore::SVModuleOp>::printAssembly(mlir::Operation*, mlir::OpAsmPrinter&, llvm::StringRef) /home/uenoku/dev/circt/llvm/mlir/include/mlir/IR/OperationSupport.h:555:39
#18 0x000062a9aa3eef66 llvm::ilist_node_base<true>::getNext() const /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/ilist_node_base.h:43:45
#19 0x000062a9aa3eef66 llvm::ilist_node_impl<llvm::ilist_detail::node_options<mlir::Operation, true, false, void, false>>::getNext() /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/ilist_node.h:94:66
#20 0x000062a9aa3eef66 llvm::ilist_iterator<llvm::ilist_detail::node_options<mlir::Operation, true, false, void, false>, false, false>::operator++() /home/uenoku/dev/circt/llvm/llvm/include/llvm/ADT/ilist_iterator.h:157:25
#21 0x000062a9aa3eef66 (anonymous namespace)::DummyAliasOperationPrinter::print(mlir::Block*, bool, bool) /home/uenoku/dev/circt/llvm/mlir/lib/IR/AsmPrinter.cpp:727:26
|
Thanks for catching this @uenoku 😍! I'll take another look to fix this. |
Add a missing check for absent port location information to `printModuleSignatureNew`. This would cause crashes if the `--mlir-print-debuginfo` option was set during emission and there are not port locations available. This mirrors what we already do for port attributes. Thanks @uenoku for pointing this out in #7112.
Add module types and support for ports to the `moore.module` and `moore.instance` operations. Also add a `moore.output` operation as a terminator for modules, similar to the HW dialect. Extend ImportVerilog to generate ports. The ports on SVModuleOp follow a similar strategy as in HWModuleOp: all input, inout, and ref ports are passed to the module as inputs and are carried inside the module as block arguments, while output ports are assigned by the OutputOp terminator. The Moore dialect reuses the `hw::ModuleType` but does not use the `inout` direction. Instead, inout ports will be represented as inputs with a `net<T>` wrapper type, while ref ports will be wrapped as `ref<T>`. Instances work identically to the HW dialect: input, inout, and ref port connections are carried as operands, while output ports are represented as results. This commit also adds module and instance port support to the ImportVerilog conversion. Regular ports are mapped to corresponding ports on the module with the appropriate direction. Multi-ports, which are a weird quirk of SystemVerilog that allow multiple ports to be grouped up and presented to the outside as a single named port, are split up into individual module ports. This is necessary since this group can contain ports of different directions. Inside a module Slang automatically generates local net or variable declarations for all ports. The user may specify these declarations themselves when using non-ANSI port lists, which Slang handles properly. ImportVerilog inserts appropriate continuous assignments to drive the actual input port value onto the local net or variable declaration, and to drive the local declaration's value onto the actual output port of the module. This properly adapts from SystemVerilog's assignable and connectable ports that feel like nets or variables, to the Moore dialect's by-value passing of inputs and outputs. Instances in Slang have expressions connected to their ports. Input ports lower this expression and directly use the result as an operand. Output ports lower this expression and drive it through a continuous assignment inserted after the instance, with the instance's corresponding result on the right-hand side of the assignment. Once we have a `ref<T>` type, and later potentially also a `net<T>` type, the port lowering shall be revisited to ensure that inout and ref ports are mapped to net and ref types, respectively. The lowering of expressions connected to ports requires more care to ensure that they are appropriately lowered to lvalues or rvalues, as needed by the port direction. A `moore.short_circuit` operation or similar would help to connect inout ports to the local net declarations in a module, and to capture `alias` statements. --------- Co-authored-by: cepheus <buyun.xu@terapines.com> Co-authored-by: Hailong Sun <hailong.sun@terapines.com>
fabianschuiki
force-pushed
the
fschuiki/moore-hierarchy
branch
from
76a758e
to
abe4b03
Compare
Add module types and support for ports to the
moore.moduleandmoore.instanceoperations. Also add amoore.outputoperation as a terminator for modules, similar to the HW dialect. Extend ImportVerilog to generate ports.The ports on SVModuleOp follow a similar strategy as in HWModuleOp: all input, inout, and ref ports are passed to the module as inputs and are carried inside the module as block arguments, while output ports are assigned by the OutputOp terminator. The Moore dialect reuses the
hw::ModuleTypebut does not use theinoutdirection. Instead, inout ports will be represented as inputs with anet<T>wrapper type, while ref ports will be wrapped asref<T>.Instances work identically to the HW dialect: input, inout, and ref port connections are carried as operands, while output ports are represented as results.
This commit also adds module and instance port support to the ImportVerilog conversion. Regular ports are mapped to corresponding ports on the module with the appropriate direction. Multi-ports, which are a weird quirk of SystemVerilog that allow multiple ports to be grouped up and presented to the outside as a single named port, are split up into individual module ports. This is necessary since this group can contain ports of different directions.
Inside a module Slang automatically generates local net or variable declarations for all ports. The user may specify these declarations themselves when using non-ANSI port lists, which Slang handles properly. ImportVerilog inserts appropriate continuous assignments to drive the actual input port value onto the local net or variable declaration, and to drive the local declaration's value onto the actual output port of the module. This properly adapts from SystemVerilog's assignable and connectable ports that feel like nets or variables, to the Moore dialect's by-value passing of inputs and outputs.
Instances in Slang have expressions connected to their ports. Input ports lower this expression and directly use the result as an operand. Output ports lower this expression and drive it through a continuous assignment inserted after the instance, with the instance's corresponding result on the right-hand side of the assignment.
Once we have a
ref<T>type, and later potentially also anet<T>type, the port lowering shall be revisited to ensure that inout and ref ports are mapped to net and ref types, respectively. The lowering of expressions connected to ports requires more care to ensure that they are appropriately lowered to lvalues or rvalues, as needed by the port direction. Amoore.short_circuitoperation or similar would help to connect inout ports to the local net declarations in a module, and to capturealiasstatements.