[ImportVerilog] Preserve declaration order across regular and interface-modport ports

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<unsigned> outputIdx;
+  std::optional<unsigned> 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<unsigned> outputIdx;
+  std::optional<unsigned> inputIdx;
 };
 
 /// Lowering information for an expanded interface instance. Maps each interface
@@ -105,6 +113,10 @@ struct ModuleLowering {
   SmallVector<FlattenedIfacePort> ifacePorts;
   DenseMap<const slang::syntax::SyntaxNode *, const slang::ast::PortSymbol *>
       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`

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<Value> inputValues;
-    SmallVector<Value> 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<Value> inputValues(moduleLowering->numExplicitInputs);
+    SmallVector<Value> 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<moore::RefType>(val.getType()) && !isa<moore::RefType>(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,19 +1065,22 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
         return failure();
       auto portName = builder.getStringAttr(port.name);
       BlockArgument arg;
+      std::optional<unsigned> portOutputIdx;
+      std::optional<unsigned> 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.
         if (port.direction != ArgumentDirection::In)
           type = moore::RefType::get(cast<moore::UnpackedType>(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<unsigned> ifaceOutputIdx;
+          std::optional<unsigned> 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<moore::UnpackedType>(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<moore::UnpackedType>(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<Value> outputs;
+  // terminator. Outputs are placed by slot index so regular and
+  // interface-modport outputs interleave in declaration order.
+  SmallVector<Value> 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<moore::RefType>(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

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