Skip to content

Commit

Permalink
[FIRRTL][GCSM] Use buffer wires for forced output ports (#3431)
Browse files Browse the repository at this point in the history 
We use buffer wires to break the verilog net when using force
statements.  So far we have only seen a need to do this for input ports,
but the same situation can happen with output ports.  When we are
forcing the value of an output port, we need to insert a wire inside the
module between it and submodule ports, so that the submodule's output
port is not also forced.

Co-authored-by: Will Dietz <[email protected]>
  • Loading branch information
@youngar @dtzSiFive
youngar and dtzSiFive authored Jun 29, 2022
1 parent b996b2e commit b560c21
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 53 deletions.
97 changes: 61 additions & 36 deletions lib/Dialect/FIRRTL/Transforms/GrandCentralSignalMappings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ using namespace firrtl;
// Signal driver annotations are needed when processing both circuits:
//
// First the original annotations are used with the main circuit, in order to
// emit extra wires around forced inputs and to emit updated annotations
// emit extra wires around forced ports and to emit updated annotations
// pointing to the values across naming or hierarchy changes performed.
//
// Second these updated annotations are used when processing the subcircuit
Expand Down Expand Up @@ -123,7 +123,7 @@ static T &operator<<(T &os, const SignalMapping &mapping) {
/// dictated by the presence of `SignalDriverAnnotation` on the module and
/// individual operations inside it.
/// If the module is the "remote" (main) circuit, gather those mappings
/// for use handling forced input ports and creating updated mappings.
/// for use handling forced ports and creating updated mappings.
void ModuleSignalMappings::run() {
// Check whether this module has any `SignalDriverAnnotation`s. These indicate
// whether the module contains any operations with such annotations and
Expand Down Expand Up @@ -348,8 +348,8 @@ struct ExtModules {
SmallVector<FExtModuleOp> json;
};

/// Information gathered about mappings, used for identifying forced input ports
/// and generating updated mappings for the remote side (main circuit).
/// Information gathered about mappings, used for identifying forced ports and
/// generating updated mappings for the remote side (main circuit).
struct ModuleMappingResult {
/// Were changes made that invalidate analyses?
bool allAnalysesPreserved;
Expand All @@ -368,7 +368,7 @@ class GrandCentralSignalMappingsPass
StringRef outputFilename,
const ExtModules &extmodules);

/// Fixup forced input ports and emit updated mappings
/// Fixup forced ports and emit updated mappings.
/// Used when processing the main (remote) circuit.
FailureOr<bool>
emitUpdatedMappings(CircuitOp circuit, StringAttr circuitPackage,
Expand Down Expand Up @@ -451,7 +451,7 @@ void GrandCentralSignalMappingsPass::runOnOperation() {

// If this is a subcircuit, then emit JSON information necessary to drive
// SiFive tools.
// Otherwise handle any forced input ports and emit updated mappings.
// Otherwise handle any forced ports and emit updated mappings.
if (isSubCircuit) {
emitSubCircuitJSON(circuit, circuitPackage, outputFilename, extmodules);
} else {
Expand Down Expand Up @@ -544,29 +544,75 @@ FailureOr<bool> GrandCentralSignalMappingsPass::emitUpdatedMappings(
}
}

// (2) Find input ports that are sinks, insert wires at instantiation points
// (2) Find ports that are sinks, insert buffer wires to break the net.
// Input ports are buffered at instantiation points, outputs in the module.
auto *instanceGraph = &getAnalysis<InstanceGraph>();
DenseSet<unsigned> forcedInputPorts;
llvm::SmallVector<unsigned, 32> forcedInputPorts;
llvm::SmallVector<unsigned, 32> forcedOutputPorts;
for (auto &[_, mod, mappings] : results) {
// Walk mappings looking for module ports that are being driven,
// and gather their indices for fixing up.
forcedInputPorts.clear();
forcedOutputPorts.clear();
for (auto &mapping : mappings) {
if (mapping.dir == MappingDirection::DriveRemote /* Sink */) {
if (auto blockArg = mapping.localValue.dyn_cast<BlockArgument>()) {
auto portIdx = blockArg.getArgNumber();
// Port may be driven multiple times along different instance paths
if (mod.getPortDirection(portIdx) == Direction::In) {
// Port may be driven multiple times along different instance paths
forcedInputPorts.insert(portIdx);
forcedInputPorts.push_back(portIdx);
} else {
forcedOutputPorts.push_back(portIdx);
}
}
}
}
llvm::sort(forcedInputPorts);
forcedInputPorts.erase(
std::unique(forcedInputPorts.begin(), forcedInputPorts.end()),
forcedInputPorts.end());
llvm::sort(forcedOutputPorts);
forcedOutputPorts.erase(
std::unique(forcedOutputPorts.begin(), forcedOutputPorts.end()),
forcedOutputPorts.end());

// Replace uses of each driven port with a wire that's connected to a wire
// that is connected to the port. This is done to cause an 'assign' to be
// created, disconnecting the forced port's net from its uses.
auto breakNet = [modName = mod.moduleName()](
OpBuilder &builder, Value port,
ModuleNamespace &moduleNamespace, StringRef portName) {
// Create chain like:
// port_result <= foo_dataIn_x_buffer
// foo_dataIn_x_buffer <= foo_dataIn_x
auto replacementWireName = builder.getStringAttr(
moduleNamespace.newName(modName + "_" + portName));
auto bufferWireName = builder.getStringAttr(
moduleNamespace.newName(replacementWireName.getValue() + "_buffer"));
auto bufferWire =
builder.create<WireOp>(builder.getUnknownLoc(), port.getType(),
bufferWireName, NameKindEnum::DroppableName,
builder.getArrayAttr({}), bufferWireName);
auto replacementWire = builder.create<WireOp>(
builder.getUnknownLoc(), port.getType(), replacementWireName,
NameKindEnum::DroppableName, builder.getArrayAttr({}),
replacementWireName);
port.replaceAllUsesWith(replacementWire);
builder.create<StrictConnectOp>(builder.getUnknownLoc(), port,
bufferWire);
builder.create<StrictConnectOp>(builder.getUnknownLoc(), bufferWire,
replacementWire);
};

if (!forcedOutputPorts.empty()) {
ModuleNamespace &moduleNamespace = getModuleNamespace(mod);
auto builder = OpBuilder::atBlockBegin(mod.getBody());
for (auto portIdx : forcedOutputPorts) {
auto port = mod.getArgument(portIdx);
breakNet(builder, port, moduleNamespace, mod.getPortName(portIdx));
}
}

// Find all instantiations of this module, and replace uses of each driven
// port with a wire that's connected to a wire that is connected to the
// port. This is done to cause an 'assign' to be created, disconnecting
// the forced input port's net from its uses.
if (forcedInputPorts.empty())
continue;

Expand All @@ -581,28 +627,7 @@ FailureOr<bool> GrandCentralSignalMappingsPass::emitUpdatedMappings(

for (auto portIdx : forcedInputPorts) {
auto port = inst->getResult(portIdx);

// Create chain like:
// port_result <= foo_dataIn_x_buffer
// foo_dataIn_x_buffer <= foo_dataIn_x
auto replacementWireName =
builder.getStringAttr(moduleNamespace.newName(
mod.moduleName() + "_" + mod.getPortName(portIdx)));
auto bufferWireName = builder.getStringAttr(moduleNamespace.newName(
replacementWireName.getValue() + "_buffer"));
auto bufferWire =
builder.create<WireOp>(builder.getUnknownLoc(), port.getType(),
bufferWireName, NameKindEnum::DroppableName,
builder.getArrayAttr({}), bufferWireName);
auto replacementWire = builder.create<WireOp>(
builder.getUnknownLoc(), port.getType(), replacementWireName,
NameKindEnum::DroppableName, builder.getArrayAttr({}),
replacementWireName);
port.replaceAllUsesWith(replacementWire);
builder.create<StrictConnectOp>(builder.getUnknownLoc(), port,
bufferWire);
builder.create<StrictConnectOp>(builder.getUnknownLoc(), bufferWire,
replacementWire);
breakNet(builder, port, moduleNamespace, mod.getPortName(portIdx));
}
}
}
Expand Down
43 changes: 26 additions & 17 deletions test/Dialect/FIRRTL/grand-central-signal-mappings.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,26 +199,27 @@ firrtl.circuit "RemoveDrivers" attributes {

// -----

// Check that GCT-SM generates 2 dummy wires for any input ports which are
// forced. This is done to work around the way that SystemVerilog force
// statements work. If an input port is forced, this will force that entire
// net. If there is NOT a wire used at the connection of the input port, the
// effect of the force can be extremely far-reaching. The Scala-based FIRRTL
// Compiler (SFC) _always_ emits a wire and never saw this problem.
// Two wires are used so an 'assign' is created, which is what breaks the net.
// Specifically, check the following things:
// 1. An input port that is forced gets the 2 dummy wires.
// 2. An output port that is forced does NOT get dummy wires.
// Check that GCT-SM generates 2 dummy wires for any ports which are forced.
// This is done to work around the way that SystemVerilog force statements
// work. If a port is forced, this will force that entire net. If there is
// NOT a wire used at the connection of the port, the effect of the force can
// be extremely far-reaching. The Scala-based FIRRTL Compiler (SFC) _always_
// emits a wire and never saw this problem. Two wires are used so an 'assign'
// is created, which is what breaks the net.
//
// CHECK-LABEL: firrtl.circuit "AddWireToForcedInputs"
firrtl.circuit "AddWireToForcedInputs" attributes {
// For forced input ports, the buffer wires are created at the instantiation
// site. For forced output ports, the buffer wires are created inside the
// module.
// CHECK-LABEL: firrtl.circuit "AddWireToForcedPorts"
firrtl.circuit "AddWireToForcedPorts" attributes {
annotations = [
{annotations = [],
circuit = "circuit empty :\0A module empty :\0A\0A skip\0A",
circuitPackage = "driving",
class = "sifive.enterprise.grandcentral.SignalDriverAnnotation",
isSubCircuit = false,
id = 0 : i64}]} {
// CHECK: firrtl.module private @ForcedPort
firrtl.module private @ForcedPort(
in %in: !firrtl.uint<1> sym @in [
{class = "sifive.enterprise.grandcentral.SignalDriverAnnotation",
Expand All @@ -234,15 +235,23 @@ firrtl.circuit "AddWireToForcedInputs" attributes {
peer = "~signal_driver|signal_driver>out_sink",
side = "remote",
targetId = 4 : i64}]) attributes {
annotations = [{class = "sifive.enterprise.grandcentral.SignalDriverAnnotation", id = 0 : i64}]} {}
// CHECK: firrtl.module @AddWireToForcedInputs
firrtl.module @AddWireToForcedInputs(in %in: !firrtl.uint<1>, out %out: !firrtl.uint<1>) attributes {
annotations = [{class = "sifive.enterprise.grandcentral.SignalDriverAnnotation", id = 0 : i64}]} {
// CHECK-NEXT: %[[buffer_wire:.+]] = firrtl.wire sym @{{.+}}
// CHECK-NEXT: %[[port_wire:.+]] = firrtl.wire sym @{{.+}}
// CHECK-NEXT: firrtl.strictconnect %out, %[[buffer_wire]]
// CHECK-NEXT: firrtl.strictconnect %[[buffer_wire]], %[[port_wire]]
// CHECK-NEXT: firrtl.strictconnect %[[port_wire]], %in
firrtl.strictconnect %out, %in : !firrtl.uint<1>
// CHECK-NEXT: }
}
// CHECK: firrtl.module @AddWireToForcedPorts
firrtl.module @AddWireToForcedPorts(in %in: !firrtl.uint<1>, out %out: !firrtl.uint<1>) attributes {
annotations = [
{class = "sifive.enterprise.grandcentral.SignalDriverAnnotation", id = 0 : i64}]} {
// CHECK-NEXT: firrtl.instance sub
%sub_in, %sub_out = firrtl.instance sub @ForcedPort(in in: !firrtl.uint<1>, out out: !firrtl.uint<1>)
// CHECK-NEXT: %[[buffer_wire:.+]] = firrtl.wire sym @[[buffer_wire_sym:.+]] : !firrtl.uint<1>
// CHECK-NEXT: %[[sub_in_wire:.+]] = firrtl.wire sym @[[sub_in_wire_sym:.+]] : !firrtl.uint<1>
// CHECK-NEXT: %[[buffer_wire:.+]] = firrtl.wire sym @{{.+}} : !firrtl.uint<1>
// CHECK-NEXT: %[[sub_in_wire:.+]] = firrtl.wire sym @{{.+}} : !firrtl.uint<1>
// CHECK-NEXT: firrtl.strictconnect %sub_in, %[[buffer_wire]]
// CHECK-NEXT: firrtl.strictconnect %[[buffer_wire]], %[[sub_in_wire]]
// CHECK-NEXT: firrtl.strictconnect %[[sub_in_wire]], %in
Expand Down

0 comments on commit b560c21

Please sign in to comment.