Skip to content

Commit

Permalink
[ExportVerilog] Inline expressions into bound instances (#4574)
Browse files Browse the repository at this point in the history 
Previously expressions feed into bound instances are anchored by wires
so that we can remotely get names of wires while bindfile emission.
Since #4573 changes name legalizations and we can get local names by
inspecting IR, we can get valid emission by justing using `emitExpression`.
  • Loading branch information
@uenoku
uenoku committed Jan 24, 2023
1 parent 9dd59bc commit 1c38d70
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 119 deletions.
125 changes: 21 additions & 104 deletions lib/Conversion/ExportVerilog/ExportVerilog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,8 +696,8 @@ bool ExportVerilog::isExpressionEmittedInline(Operation *op,
// Never create a temporary which is only going to be assigned to an output
// port, wire, or reg.
if (op->hasOneUse() &&
isa<hw::OutputOp, sv::AssignOp, sv::BPAssignOp, sv::PAssignOp>(
*op->getUsers().begin()))
isa<hw::OutputOp, sv::AssignOp, sv::BPAssignOp, sv::PAssignOp,
hw::InstanceOp>(*op->getUsers().begin()))
return true;

// If mux inlining is dissallowed, we cannot inline muxes.
Expand Down Expand Up @@ -1171,9 +1171,6 @@ class ModuleEmitter : public EmitterBase {
void emitBind(BindOp op);
void emitBindInterface(BindInterfaceOp op);

StringRef getNameRemotely(Value value, const ModulePortInfo &modulePorts,
HWModuleOp remoteModule);

/// Legalize the given field name if it is an invalid verilog name.
StringRef getVerilogStructFieldName(StringAttr field) {
return fieldNameResolver.getRenamedFieldName(field).getValue();
Expand Down Expand Up @@ -4570,11 +4567,26 @@ void ModuleEmitter::emitBind(BindOp op) {
ps << "." << PPExtString(elt.getName());
ps.nbsp(maxNameLength - elt.getName().size());
ps << " (";
llvm::SmallPtrSet<Operation *, 4> ops;
if (elt.isOutput()) {
assert(portVal.hasOneUse() && "output port must have a single use");

if (auto output = dyn_cast_or_null<OutputOp>(
portVal.getUses().begin()->getOwner())) {
// If this is directly using the output port of the containing
// module, just specify that directly.
size_t outputPortNo = portVal.getUses().begin()->getOperandNumber();
ps << PPExtString(getPortVerilogName(
parentMod, parentMod.getOutputPort(outputPortNo)));
} else {
portVal = portVal.getUsers().begin()->getOperand(0);
ExprEmitter(*this, ops).emitExpression(portVal, LowestPrecedence);
}
} else {
ExprEmitter(*this, ops).emitExpression(portVal, LowestPrecedence);
}

// Emit the value as an expression.
auto name = getNameRemotely(portVal, parentPortInfo, parentMod);
assert(!name.empty() && "bind port connection must have a name");
ps << PPExtString(name) << ")";
ps << ")";

if (isZeroWidth)
ps << PP::end; // Close never-break group.
Expand All @@ -4586,101 +4598,6 @@ void ModuleEmitter::emitBind(BindOp op) {
setPendingNewline();
}

/// Return the name of a value in a remote module to be used in a `bind`
/// statement. This function examines the remote module `remoteModule` and looks
/// up the corresponding name. This requires that all names this function may be
/// asked to lookup have been legalized in pre-pass.
StringRef ModuleEmitter::getNameRemotely(Value value,
const ModulePortInfo &modulePorts,
HWModuleOp remoteModule) {
if (auto barg = value.dyn_cast<BlockArgument>())
return getPortVerilogName(remoteModule,
modulePorts.inputs[barg.getArgNumber()]);

Operation *valueOp = value.getDefiningOp();

// Handle wires/registers/XMR references, likely as instance inputs.
if (auto readinout = dyn_cast<ReadInOutOp>(valueOp)) {
auto *wireInput = readinout.getInput().getDefiningOp();
if (!wireInput)
return {};
if (isa<WireOp, RegOp, LogicOp>(wireInput))
return getSymOpName(wireInput);

if (auto xmr = dyn_cast<XMROp>(wireInput)) {
SmallString<16> xmrString;
if (xmr.getIsRooted())
xmrString.append("$root.");
for (auto s : xmr.getPath()) {
xmrString.append(s.cast<StringAttr>().getValue());
xmrString.append(".");
}
xmrString.append(xmr.getTerminal());
return StringAttr::get(value.getContext(), xmrString);
}

// TODO: This shares a lot of code with the XMRRefOp visitor. Combine these
// to share logic.
if (auto xmrRef = dyn_cast<XMRRefOp>(wireInput)) {
SmallString<32> xmrString;
auto refAttr = xmrRef.getRefAttr();

if (auto innerRef = dyn_cast<InnerRefAttr>(refAttr)) {
auto ref = state.symbolCache.getInnerDefinition(innerRef.getModule(),
innerRef.getName());
auto *module = state.symbolCache.getDefinition(innerRef.getModule());
xmrString.append(getSymOpName(module));
xmrString.append(".");
if (ref.hasPort())
xmrString.append(getPortVerilogName(ref.getOp(), ref.getPort()));
else
xmrString.append(getSymOpName(ref.getOp()));
} else {

auto globalRef = cast<hw::HierPathOp>(state.symbolCache.getDefinition(
cast<FlatSymbolRefAttr>(xmrRef.getRefAttr()).getAttr()));
auto namepath = globalRef.getNamepathAttr().getValue();
auto *module = state.symbolCache.getDefinition(
cast<InnerRefAttr>(namepath.front()).getModule());
xmrString.append(getSymOpName(module));
for (auto sym : namepath) {
xmrString.append(".");
auto innerRef = cast<InnerRefAttr>(sym);
auto ref = state.symbolCache.getInnerDefinition(innerRef.getModule(),
innerRef.getName());
if (ref.hasPort()) {
xmrString.append(getPortVerilogName(ref.getOp(), ref.getPort()));
continue;
}
xmrString.append(getSymOpName(ref.getOp()));
}
}
auto leaf = xmrRef.getStringLeafAttr();
if (leaf && leaf.size())
xmrString.append(leaf);
return StringAttr::get(xmrRef.getContext(), xmrString);
}
}

// Handle values being driven onto wires, likely as instance outputs.
if (isa<InstanceOp>(valueOp)) {
for (auto &use : value.getUses()) {
Operation *user = use.getOwner();
if (!isa<AssignOp>(user) || use.getOperandNumber() != 1)
continue;
Value drivenOnto = user->getOperand(0);
Operation *drivenOntoOp = drivenOnto.getDefiningOp();
if (isa<WireOp, RegOp, LogicOp>(drivenOntoOp))
return getSymOpName(drivenOntoOp);
}
}

// Handle local parameters.
if (isa<LocalParamOp>(valueOp))
return getSymOpName(valueOp);
return {};
}

void ModuleEmitter::emitBindInterface(BindInterfaceOp op) {
if (hasSVAttributes(op))
emitError(op, "SV attributes emission is unimplemented for the op");
Expand Down
10 changes: 5 additions & 5 deletions lib/Conversion/ExportVerilog/PrepareForEmission.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,8 @@ bool EmittedExpressionStateManager::shouldSpillWireBasedOnState(Operation &op) {
// to a wire.
if (op.hasOneUse()) {
auto *singleUser = *op.getUsers().begin();
if (isa<hw::OutputOp, sv::AssignOp, sv::BPAssignOp>(singleUser))
if (isa<hw::OutputOp, sv::AssignOp, sv::BPAssignOp, hw::InstanceOp>(
singleUser))
return false;

// If the single user is bitcast, we check the same property for the bitcast
Expand Down Expand Up @@ -705,10 +706,9 @@ static LogicalResult legalizeHWModule(Block &block,
if (auto instance = dyn_cast<InstanceOp>(op)) {
// Anchor return values to wires early
lowerInstanceResults(instance);
// Anchor ports of instances when the instance is bound by bind op, or
// forced by the option.
if (instance->hasAttr("doNotPrint") ||
options.disallowExpressionInliningInPorts)
// Anchor ports of instances when `disallowExpressionInliningInPorts` is
// enabled.
if (options.disallowExpressionInliningInPorts)
spillWiresForInstanceInputs(instance);
}

Expand Down
50 changes: 42 additions & 8 deletions test/Conversion/ExportVerilog/sv-dialect.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,32 @@ hw.module @wait_order() {
}

hw.module.extern @MyExtModule(%in: i8)
hw.module.extern @ExtModule(%in: i8) -> (out: i8)

// CHECK-LABEL: module InlineBind
// CHEC: output wire_0
hw.module @InlineBind(%a_in: i8) -> (wire: i8){
// CHECK: wire [7:0] _ext1_out;
// CHECK-NEXT: wire [7:0] _GEN;
// CHECK-NEXT: /* This instance is elsewhere emitted as a bind statement.
// CHECK-NEXT: ExtModule ext1 (
// CHECK-NEXT: .in (8'(a_in + _GEN)),
// CHECK-NEXT: .out (_ext1_out)
// CHECK-NEXT: );
// CHECK-NEXT: */
// CHECK-NEXT: /* This instance is elsewhere emitted as a bind statement.
// CHECK-NEXT: ExtModule ext2 (
// CHECK-NEXT: .in (_ext1_out),
// CHECK-NEXT: .out (wire_0)
// CHECK-NEXT: );
// CHECK-NEXT: */
%0 = sv.wire : !hw.inout<i8>
%1 = sv.read_inout %0: !hw.inout<i8>
%2 = comb.add %a_in, %1 : i8
%3 = hw.instance "ext1" sym @foo1 @ExtModule(in: %2: i8) -> (out: i8) {doNotPrint=1}
%4 = hw.instance "ext2" sym @foo2 @ExtModule(in: %3: i8) -> (out: i8) {doNotPrint=1}
hw.output %4: i8
}

// CHECK-LABEL: module MoveInstances
hw.module @MoveInstances(%a_in: i8) -> (outc : i8){
Expand Down Expand Up @@ -1403,10 +1429,7 @@ hw.module @remoteInstDut(%i: i1, %j: i1, %z: i0) -> () {
hw.instance "a1" sym @bindInst @extInst(_h: %mywire_rd: i1, _i: %myreg_rd: i1, _j: %j: i1, _k: %0: i1, _z: %z: i0) -> () {doNotPrint=1}
hw.instance "a2" sym @bindInst2 @extInst(_h: %mywire_rd: i1, _i: %myreg_rd: i1, _j: %j: i1, _k: %0: i1, _z: %z: i0) -> () {doNotPrint=1}
hw.instance "signed" sym @bindInst3 @extInst2(signed: %mywire_rd1 : i1, _i: %myreg_rd1 : i1, _j: %j: i1, _k: %0: i1, _z: %z: i0) -> () {doNotPrint=1}
// CHECK: wire _signed__k
// CHECK-NEXT: wire _a2__k = 1'h1;
// CHECK-NEXT: wire _a1__k = 1'h1;
// CHECK-NEXT: wire mywire
// CHECK: wire mywire
// CHECK-NEXT: myreg
// CHECK-NEXT: wire signed_0
// CHECK-NEXT: reg output_0
Expand Down Expand Up @@ -1711,14 +1734,14 @@ hw.module @bindInMod() {
// CHECK-NEXT: ._h (mywire),
// CHECK-NEXT: ._i (myreg),
// CHECK-NEXT: ._j (j),
// CHECK-NEXT: ._k (_a1__k)
// CHECK-NEXT: ._k (1'h1)
// CHECK-NEXT: //._z (z)
// CHECK-NEXT: );
// CHECK-NEXT: bind remoteInstDut extInst2 signed_1 (
// CHECK-NEXT: .signed_0 (signed_0),
// CHECK-NEXT: ._i (output_0),
// CHECK-NEXT: ._j (j),
// CHECK-NEXT: ._k (_signed__k)
// CHECK-NEXT: ._k (1'h1)
// CHECK: endmodule

sv.bind <@wait_order::@baz>
Expand All @@ -1734,7 +1757,7 @@ sv.bind #hw.innerNameRef<@remoteInstDut::@bindInst2>
// CHECK-NEXT: ._h (mywire),
// CHECK-NEXT: ._i (myreg),
// CHECK-NEXT: ._j (j),
// CHECK-NEXT: ._k (_a2__k)
// CHECK-NEXT: ._k (1'h1)
// CHECK-NEXT: //._z (z)
// CHECK-NEXT: );

Expand All @@ -1749,10 +1772,21 @@ hw.module @NastyPort(%.lots$of.dots: i1) -> (".more.dots": i1) {
}
sv.bind #hw.innerNameRef<@NastyPortParent::@foo>
// CHECK-LABEL: bind NastyPortParent NastyPort foo (
// CHECK-NEXT: ._lots24of_dots (_foo__lots24of_dots)
// CHECK-NEXT: ._lots24of_dots (1'h0)
// CHECK-NEXT: ._more_dots (_foo__more_dots)
// CHECK-NEXT: );

sv.bind #hw.innerNameRef<@InlineBind::@foo1>
sv.bind #hw.innerNameRef<@InlineBind::@foo2>
// CHECK-LABEL: bind InlineBind ExtModule ext1 (
// CHECK-NEXT: .in (8'(a_in + _GEN))
// CHECK-NEXT: .out (_ext1_out)
// CHECK-NEXT: );
// CHECK-LABEL: bind InlineBind ExtModule ext2 (
// CHECK-NEXT: .in (_ext1_out)
// CHECK-NEXT: .out (wire_0)
// CHECK-NEXT: );

// CHECK-LABEL: hw.module @issue595
// CHECK: sv.wire {hw.verilogName = "_GEN"} : !hw.inout<i32>

Expand Down
4 changes: 2 additions & 2 deletions test/Conversion/ExportVerilog/verilog-basic.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ hw.module @SiFive_MulDiv(%clock: i1, %reset: i1) -> (io_req_ready: i1) {
hw.probe @unused, %false, %reset, %clock: i1,i1,i1
hw.output %false : i1
// CHECK: bind_rename_port InvisibleBind_assert (
// CHECK-NEXT: ._io_req_ready_output (_InvisibleBind_assert__io_req_ready_output),
// CHECK-NEXT: ._io_req_ready_output (1'h0),
// CHECK-NEXT: .resetSignalName (reset),
// CHECK-NEXT: .clock (clock)
// CHECK-NEXT: );
Expand Down Expand Up @@ -654,6 +654,6 @@ hw.module @BindInterface() -> () {

sv.bind #hw.innerNameRef<@SiFive_MulDiv::@__ETC_SiFive_MulDiv_assert>
// CHECK-LABEL: bind SiFive_MulDiv bind_rename_port InvisibleBind_assert
// CHECK-NEXT: ._io_req_ready_output (_InvisibleBind_assert__io_req_ready_output)
// CHECK-NEXT: ._io_req_ready_output (1'h0)
// CHECK-NEXT: .resetSignalName (reset),
// CHECK-NEXT: .clock (clock)

0 comments on commit 1c38d70

Please sign in to comment.