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..eebda4ed1287 100644 --- a/lib/Conversion/ImportVerilog/Structure.cpp +++ b/lib/Conversion/ImportVerilog/Structure.cpp @@ -585,17 +585,17 @@ 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); + outputValues[*port.outputIdx] = value; else - inputValues.push_back(value); + inputValues[*port.inputIdx] = value; } // Resolve flattened interface port values. For each flattened port, @@ -628,19 +628,22 @@ struct ModuleVisitor : public BaseVisitor { } Value val = valIt->second; if (fp.direction == hw::ModulePort::Output) { - outputValues.push_back(val); + 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); + 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 +1065,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 +1077,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 +1108,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 +1156,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 +1183,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 +1346,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 +1367,7 @@ 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); + outputs[*port.outputIdx] = value; continue; } @@ -1379,7 +1391,8 @@ 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()); + 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