Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/Conversion/ImportVerilog/HierarchicalNames.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ struct InstBodyVisitor
if (currentInstBody == outermostInstBody)
return;

// References resolved via an interface port (e.g. `bus.member` where `bus`
// is an `Iface.modport` port) are not cross-instance hierarchical accesses;
// they are handled by the interface port lowering machinery in
// Structure.cpp. Recording them here would add a spurious hierPath input
// to the module signature that nothing fills in at the instance site.
if (expr.ref.isViaIfacePort())
return;

auto hierName = builder.getStringAttr(expr.symbol.name);
const slang::ast::InstanceBodySymbol *parentInstBody = nullptr;

Expand Down
6 changes: 6 additions & 0 deletions lib/Conversion/ImportVerilog/ImportVerilogInternals.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ struct FlattenedIfacePort {
/// The connected interface instance backing this port (if any). This enables
/// materializing virtual interface handles from interface ports.
const slang::ast::InstanceSymbol *ifaceInstance = nullptr;
/// For modport-typed iface ports, the ModportPortSymbol this was flattened
/// from. Slang resolves in-body accesses like `bus.member` to a
/// `HierarchicalValueExpression` whose symbol is the `ModportPortSymbol`,
/// 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;
};

/// Lowering information for an expanded interface instance. Maps each interface
Expand Down
17 changes: 13 additions & 4 deletions lib/Conversion/ImportVerilog/Structure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
}
lowering.ifacePorts.push_back({name, dir, type, portLoc, arg,
&ifacePort, mpp->internalSymbol,
ifaceInst});
ifaceInst, mpp});
}
} else {
// No modport: iterate interface body for all variables and nets.
Expand Down Expand Up @@ -1291,18 +1291,27 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) {
if (!valueSym)
continue;

Value portValue;
if (fp.direction == hw::ModulePort::Output) {
// Output interface ports are not referenceable within the module body.
// Create internal variables for them and return their value through the
// module terminator.
auto varOp = moore::VariableOp::create(
portValue = moore::VariableOp::create(
builder, fp.loc,
moore::RefType::get(cast<moore::UnpackedType>(fp.type)), fp.name,
Value());
valueSymbols.insert(valueSym, varOp);
} else {
valueSymbols.insert(valueSym, fp.arg);
portValue = fp.arg;
}
valueSymbols.insert(valueSym, portValue);
// Slang resolves in-body accesses (e.g. `bus.r`) through the
// ModportPortSymbol rather than the interface body's variable. Register
// both so the body-level expression lookup finds this port.
if (auto *mppSym = fp.modportPortSym
? fp.modportPortSym->as_if<slang::ast::ValueSymbol>()
: nullptr;
mppSym && mppSym != valueSym)
valueSymbols.insert(mppSym, portValue);
Comment on lines +1310 to +1314
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (auto *mppSym = fp.modportPortSym
? fp.modportPortSym->as_if<slang::ast::ValueSymbol>()
: nullptr;
mppSym && mppSym != valueSym)
valueSymbols.insert(mppSym, portValue);
slang::ast::ValueSymbol *mppSym = nullptr;
if (fp.modportPortSym) {
mppSym = fp.modportPortSym->as_if<slang::ast::ValueSymbol>();
}
if (mppSym && mppSym != valueSym) {
valueSymbols.insert(mppSym, portValue);
}

nit: Might be nice to take this apart, looks like that's three checks rolled into one statement


if (!fp.ifaceInstance)
continue;
Expand Down
35 changes: 35 additions & 0 deletions test/Conversion/ImportVerilog/interface-modport-body-access.sv
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// RUN: circt-verilog --ir-moore %s | FileCheck %s
// REQUIRES: slang

// Test for in-body access to modport members of a modport-typed interface port.
// The receiving module reads `bus.x` (modport input) and writes `bus.y`
// (modport output). Slang resolves these as HierarchicalValueExpressions whose
// symbol is the ModportPortSymbol; ImportVerilog must resolve them through the
// flattened interface port instead of registering them as cross-instance
// hierPath inputs (which would add spurious unfilled module ports).
// UNSUPPORTED: valgrind

interface IfaceModportBody;
logic [7:0] x;
logic [7:0] y;
modport mp (input x, output y);
endinterface

// CHECK-LABEL: moore.module private @ModportBodyUse(in %bus_x : !moore.l8, out bus_y : !moore.l8) {
// CHECK: moore.output %bus_x : !moore.l8
// CHECK: }
module ModportBodyUse(IfaceModportBody.mp bus);
assign bus.y = bus.x;
endmodule

// CHECK-LABEL: moore.module @TopModportBodyUse() {
// CHECK: %ifc_x = moore.variable : <l8>
// CHECK: %ifc_y = moore.assigned_variable %dut.bus_y : l8
// CHECK: [[X:%.+]] = moore.read %ifc_x : <l8>
// CHECK: %dut.bus_y = moore.instance "dut" @ModportBodyUse(bus_x: [[X]]: !moore.l8) -> (bus_y: !moore.l8)
// CHECK: moore.output
// CHECK: }
module TopModportBodyUse;
IfaceModportBody ifc();
ModportBodyUse dut(.bus(ifc));
endmodule