From 08c4bc1b94fddf58075e06624fe5249d53d24beb Mon Sep 17 00:00:00 2001 From: Michael Rogenmoser Date: Tue, 5 May 2026 14:28:00 -0700 Subject: [PATCH 1/2] [ImportVerilog] Preserve declaration order across regular and interface-modport ports When a module's port list interleaves an interface-modport port with regular outputs, the instance-port plumbing produced `moore.instance` / `moore.output` verifier failures of the form `operand N (...) does not match input/output type (...) of module @M`. `convertModuleHeader` walked the AST port list in order and pushed each port (regular or expanded interface-modport) to `modulePorts` interleaved by declaration order. But `convertModuleBody` built the `moore.output` operand vector by walking `lowering.ports` first (regular outputs only) and then `lowering.ifacePorts` (interface outputs only), and the visit-instance code did the same for the `moore.instance` operand and result vectors. Whenever the declaration order had an interface-modport output before a regular output, the operand vector and the module signature were permutations of each other and verification fired. Fix: record an output slot index (`outputIdx`) on `PortLowering` and `FlattenedIfacePort` at header time, and a matching input slot index (`inputIdx`). Track `numExplicitOutputs` / `numExplicitInputs` on `ModuleLowering`. Build `outputs` / `inputValues` / `outputValues` as fixed-size vectors and place values by slot index, both at the module's terminator and at the instance call site. Hierarchical-name ports keep their existing append-after-explicit-ports treatment. Regression test: `interface-port-order.sv` exercises a declaration order with an interface-modport output (l32) before a regular output (l64). The widths differ so any swap trips the `moore.output` terminator's verifier (the terminator is rigid, with no `materializeConversion` between operands and declared port types). Assisted-by: Claude Code:claude-opus-4-7 --- .../ImportVerilog/ImportVerilogInternals.h | 12 +++ lib/Conversion/ImportVerilog/Structure.cpp | 74 ++++++++++++------- .../ImportVerilog/interface-port-order.sv | 60 +++++++++++++++ 3 files changed, 119 insertions(+), 27 deletions(-) create mode 100644 test/Conversion/ImportVerilog/interface-port-order.sv diff --git a/lib/Conversion/ImportVerilog/ImportVerilogInternals.h b/lib/Conversion/ImportVerilog/ImportVerilogInternals.h index 7e48f44e8853..6517e89de7b9 100644 --- a/lib/Conversion/ImportVerilog/ImportVerilogInternals.h +++ b/lib/Conversion/ImportVerilog/ImportVerilogInternals.h @@ -46,6 +46,11 @@ struct PortLowering { const slang::ast::PortSymbol * Location loc; BlockArgument arg; + /// Slot index in the module signature. `outputIdx` is set for outputs; + /// `inputIdx` is set for inputs and inouts. Regular and expanded + /// interface-modport ports share one numbering per direction. + std::optional outputIdx; + std::optional inputIdx; }; /// Lowering information for a single signal flattened from an interface port. @@ -68,6 +73,9 @@ struct FlattenedIfacePort { /// not the underlying interface body variable, so we register both keys in /// `valueSymbols` to make the lookup find this port's `BlockArgument`. const slang::ast::Symbol *modportPortSym = nullptr; + /// Slot index in the module signature; see `PortLowering::outputIdx`. + std::optional outputIdx; + std::optional inputIdx; }; /// Lowering information for an expanded interface instance. Maps each interface @@ -105,6 +113,10 @@ struct ModuleLowering { SmallVector ifacePorts; DenseMap portsBySyntaxNode; + /// Number of explicit-port slots per direction. Hierarchical-name ports + /// are appended after these in the module signature. + unsigned numExplicitOutputs = 0; + unsigned numExplicitInputs = 0; }; /// Function lowering information. The `op` field holds either a `func::FuncOp` diff --git a/lib/Conversion/ImportVerilog/Structure.cpp b/lib/Conversion/ImportVerilog/Structure.cpp index 1a62ca274de9..a167ba12ded0 100644 --- a/lib/Conversion/ImportVerilog/Structure.cpp +++ b/lib/Conversion/ImportVerilog/Structure.cpp @@ -585,17 +585,20 @@ struct ModuleVisitor : public BaseVisitor { } // Match the module's ports up with the port values determined above. - SmallVector inputValues; - SmallVector outputValues; - inputValues.reserve(moduleType.getNumInputs()); - outputValues.reserve(moduleType.getNumOutputs()); + // Values are placed by slot index so regular and expanded + // interface-modport ports interleave in declaration order. + SmallVector inputValues(moduleLowering->numExplicitInputs); + SmallVector outputValues(moduleLowering->numExplicitOutputs); for (auto &port : moduleLowering->ports) { auto value = portValues.lookup(&port.ast); - if (port.ast.direction == ArgumentDirection::Out) - outputValues.push_back(value); - else - inputValues.push_back(value); + if (port.ast.direction == ArgumentDirection::Out) { + assert(port.outputIdx && "output port missing outputIdx"); + outputValues[*port.outputIdx] = value; + } else { + assert(port.inputIdx && "input port missing inputIdx"); + inputValues[*port.inputIdx] = value; + } } // Resolve flattened interface port values. For each flattened port, @@ -628,19 +631,24 @@ struct ModuleVisitor : public BaseVisitor { } Value val = valIt->second; if (fp.direction == hw::ModulePort::Output) { - outputValues.push_back(val); + assert(fp.outputIdx && "output iface port missing outputIdx"); + outputValues[*fp.outputIdx] = val; } else { // For input ports, if the value is a ref (from VariableOp/NetOp), // read it to get the rvalue, unless the port itself expects a ref. if (isa(val.getType()) && !isa(fp.type)) val = moore::ReadOp::create(builder, loc, val); - inputValues.push_back(val); + assert(fp.inputIdx && "input iface port missing inputIdx"); + inputValues[*fp.inputIdx] = val; } } - // Insert conversions for input ports. + // Insert conversions for input ports. Unfilled slots (e.g. unresolved + // interface-modport ports) are reported by the null-check loop below. for (auto [value, type] : llvm::zip(inputValues, moduleType.getInputTypes())) { + if (!value) + continue; // TODO: This should honor signedness in the conversion. value = context.materializeConversion(type, value, false, value.getLoc()); if (!value) @@ -1062,9 +1070,11 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) { return failure(); auto portName = builder.getStringAttr(port.name); BlockArgument arg; + std::optional portOutputIdx; + std::optional portInputIdx; if (port.direction == ArgumentDirection::Out) { modulePorts.push_back({portName, type, hw::ModulePort::Output}); - outputIdx++; + portOutputIdx = outputIdx++; } else { // Only the ref type wrapper exists for the time being, the net type // wrapper for inout may be introduced later if necessary. @@ -1072,9 +1082,10 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) { type = moore::RefType::get(cast(type)); modulePorts.push_back({portName, type, hw::ModulePort::Input}); arg = block->addArgument(type, portLoc); - inputIdx++; + portInputIdx = inputIdx++; } - lowering.ports.push_back({port, portLoc, arg}); + lowering.ports.push_back( + {port, portLoc, arg, portOutputIdx, portInputIdx}); return success(); }; @@ -1102,21 +1113,23 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) { builder.getStringAttr(Twine(portPrefix) + StringRef(mpp->name)); BlockArgument arg; hw::ModulePort::Direction dir; + std::optional ifaceOutputIdx; + std::optional ifaceInputIdx; if (mpp->direction == ArgumentDirection::Out) { dir = hw::ModulePort::Output; modulePorts.push_back({name, type, dir}); - outputIdx++; + ifaceOutputIdx = outputIdx++; } else { dir = hw::ModulePort::Input; if (mpp->direction != ArgumentDirection::In) type = moore::RefType::get(cast(type)); modulePorts.push_back({name, type, dir}); arg = block->addArgument(type, portLoc); - inputIdx++; + ifaceInputIdx = inputIdx++; } - lowering.ifacePorts.push_back({name, dir, type, portLoc, arg, - &ifacePort, mpp->internalSymbol, - ifaceInst, mpp}); + lowering.ifacePorts.push_back( + {name, dir, type, portLoc, arg, &ifacePort, mpp->internalSymbol, + ifaceInst, mpp, ifaceOutputIdx, ifaceInputIdx}); } } else { // No modport: iterate interface body for all variables and nets. @@ -1148,10 +1161,9 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) { auto refType = moore::RefType::get(cast(type)); modulePorts.push_back({name, refType, hw::ModulePort::Input}); auto arg = block->addArgument(refType, portLoc); - inputIdx++; - lowering.ifacePorts.push_back({name, hw::ModulePort::Input, refType, - portLoc, arg, &ifacePort, bodySym, - instSym}); + lowering.ifacePorts.push_back( + {name, hw::ModulePort::Input, refType, portLoc, arg, &ifacePort, + bodySym, instSym, nullptr, std::nullopt, inputIdx++}); } } return success(); @@ -1176,6 +1188,10 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) { } } + // Record explicit-port counts before hierarchical-name ports are appended. + lowering.numExplicitOutputs = outputIdx; + lowering.numExplicitInputs = inputIdx; + // Mapping hierarchical names into the module's ports. for (auto &hierPath : hierPaths[module]) { assert(!hierPath.valueSyms.empty() && "hierPath must have valueSyms"); @@ -1335,8 +1351,9 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) { // Create additional ops to drive input port values onto the corresponding // internal variables and nets, and to collect output port values for the - // terminator. - SmallVector outputs; + // terminator. Outputs are placed by slot index so regular and + // interface-modport outputs interleave in declaration order. + SmallVector outputs(lowering.numExplicitOutputs); for (auto &port : lowering.ports) { Value value; if (auto *expr = port.ast.getInternalExpr()) { @@ -1355,7 +1372,8 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) { if (port.ast.direction == slang::ast::ArgumentDirection::Out) { if (isa(value.getType())) value = moore::ReadOp::create(builder, value.getLoc(), value); - outputs.push_back(value); + assert(port.outputIdx && "output port missing outputIdx"); + outputs[*port.outputIdx] = value; continue; } @@ -1379,7 +1397,9 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) { Value ref = valueSymbols.lookup(valueSym); if (!ref) continue; - outputs.push_back(moore::ReadOp::create(builder, fp.loc, ref).getResult()); + assert(fp.outputIdx && "output iface port missing outputIdx"); + outputs[*fp.outputIdx] = + moore::ReadOp::create(builder, fp.loc, ref).getResult(); } // Ensure the number of operands of this module's terminator and the number of diff --git a/test/Conversion/ImportVerilog/interface-port-order.sv b/test/Conversion/ImportVerilog/interface-port-order.sv new file mode 100644 index 000000000000..cf5a902d228b --- /dev/null +++ b/test/Conversion/ImportVerilog/interface-port-order.sv @@ -0,0 +1,60 @@ +// RUN: circt-verilog --ir-moore %s | FileCheck %s +// REQUIRES: slang + +// Internal issue in Slang v3 about jump depending on uninitialised value. +// UNSUPPORTED: valgrind + +// Regression test: when a module's port list interleaves an interface modport +// output with a regular output, the operands of the `moore.output` terminator +// must follow the *declaration* order of those outputs. Previously, +// ImportVerilog appended interface-modport outputs after all regular outputs, +// causing a verifier failure when the declaration order was the opposite. +// +// The terminator is rigid (no `materializeConversion` between operand and +// declared port type), so a swap between two same-bit-count but +// different-width outputs trips the verifier. + +interface FooBus #(parameter int unsigned W = 1) (); + logic [W-1:0] data; + modport Slave (output data); +endinterface + +// The wrapper has a non-trivial output ordering: the FooBus.Slave modport +// (output `data`) appears *before* the regular `out_o` output. With distinct +// widths the verifier detects any swap. +// +// CHECK-LABEL: moore.module private @WrapIntf( +// CHECK-SAME: in %clk_i : !moore.l1 +// CHECK-SAME: out {{(bus[._]data)}} : !moore.l32 +// CHECK-SAME: out out_o : !moore.l64 +// CHECK-SAME: ) { +// First operand drives `bus.data` (l32), second drives `out_o` (l64). +// CHECK: moore.output %{{.+}}, %{{.+}} : !moore.l32, !moore.l64 +// CHECK: } +module WrapIntf ( + input logic clk_i, + FooBus.Slave bus, + output logic [63:0] out_o +); + // `bus.data` is left undriven; ImportVerilog creates a default variable + // that flows through the module's output. The point of this test is the + // declaration-order plumbing, not the modport member's body access. + assign out_o = '0; +endmodule + +// At the instance call site, the result types of `moore.instance` must also +// follow declaration order across regular and interface-modport outputs. +// +// CHECK-LABEL: moore.module @TopWrapIntf +// CHECK: %{{.*}} = moore.instance "i_w" @WrapIntf( +// CHECK-SAME: clk_i: %{{.+}}: !moore.l1 +// CHECK-SAME: ) -> ({{(bus[._]data)}}: !moore.l32, out_o: !moore.l64) +module TopWrapIntf; + logic clk; + FooBus #(.W(32)) bus (); + WrapIntf i_w ( + .clk_i(clk), + .bus(bus), + .out_o() + ); +endmodule From 19b380f3923f4bca0d6aed7d481867498d8fe7b9 Mon Sep 17 00:00:00 2001 From: Michael Rogenmoser Date: Thu, 21 May 2026 13:15:50 +0200 Subject: [PATCH 2/2] [ImportVerilog] Remove unneeded asserts --- lib/Conversion/ImportVerilog/Structure.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/Conversion/ImportVerilog/Structure.cpp b/lib/Conversion/ImportVerilog/Structure.cpp index a167ba12ded0..eebda4ed1287 100644 --- a/lib/Conversion/ImportVerilog/Structure.cpp +++ b/lib/Conversion/ImportVerilog/Structure.cpp @@ -592,13 +592,10 @@ struct ModuleVisitor : public BaseVisitor { for (auto &port : moduleLowering->ports) { auto value = portValues.lookup(&port.ast); - if (port.ast.direction == ArgumentDirection::Out) { - assert(port.outputIdx && "output port missing outputIdx"); + if (port.ast.direction == ArgumentDirection::Out) outputValues[*port.outputIdx] = value; - } else { - assert(port.inputIdx && "input port missing inputIdx"); + else inputValues[*port.inputIdx] = value; - } } // Resolve flattened interface port values. For each flattened port, @@ -631,14 +628,12 @@ struct ModuleVisitor : public BaseVisitor { } Value val = valIt->second; if (fp.direction == hw::ModulePort::Output) { - assert(fp.outputIdx && "output iface port missing outputIdx"); outputValues[*fp.outputIdx] = val; } else { // For input ports, if the value is a ref (from VariableOp/NetOp), // read it to get the rvalue, unless the port itself expects a ref. if (isa(val.getType()) && !isa(fp.type)) val = moore::ReadOp::create(builder, loc, val); - assert(fp.inputIdx && "input iface port missing inputIdx"); inputValues[*fp.inputIdx] = val; } } @@ -1372,7 +1367,6 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) { if (port.ast.direction == slang::ast::ArgumentDirection::Out) { if (isa(value.getType())) value = moore::ReadOp::create(builder, value.getLoc(), value); - assert(port.outputIdx && "output port missing outputIdx"); outputs[*port.outputIdx] = value; continue; } @@ -1397,7 +1391,6 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) { Value ref = valueSymbols.lookup(valueSym); if (!ref) continue; - assert(fp.outputIdx && "output iface port missing outputIdx"); outputs[*fp.outputIdx] = moore::ReadOp::create(builder, fp.loc, ref).getResult(); }