Skip to content

Commit

Permalink
[FIRRTL][LowerLayers] Clean up names of artifacts generated by layers
Browse files Browse the repository at this point in the history 
- Layer module name: move into helper.
- Layer instance name:
  - don't put the parent module name in the instance name,
  - lowercase each part of the instance name, not just the first word.
- Layer file name: move into helper.
- Captured port names: don't prefix the port name with an underscore.
  • Loading branch information
@rwy7
rwy7 committed Feb 21, 2024
1 parent e8f4f34 commit 6a768b2
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 65 deletions.
105 changes: 72 additions & 33 deletions lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ static bool isAncestor(Operation *op, Value value) {
return op->isAncestor(argument.getOwner()->getParentOp());
}

//===----------------------------------------------------------------------===//
// Type Conversion
//===----------------------------------------------------------------------===//

namespace {

/// Indicates the kind of reference that was captured.
Expand All @@ -63,6 +59,64 @@ struct ConnectInfo {
using InnerRefMap =
DenseMap<hw::InnerRefAttr, std::pair<hw::InnerSymAttr, StringAttr>>;

//===----------------------------------------------------------------------===//
// Naming Helpers
//===----------------------------------------------------------------------===//

static void appendName(StringRef name, SmallString<32> &output,
bool toLower = false) {
if (name.empty())
return;
if (!output.empty())
output.append("_");
output.append(name);
if (!toLower)
return;
auto i = output.size() - name.size();
output[i] = llvm::toLower(output[i]);
}

static void appendName(SymbolRefAttr name, SmallString<32> &output,
bool toLower = false) {
appendName(name.getRootReference(), output, toLower);
for (auto nested : name.getNestedReferences())
appendName(nested.getValue(), output, toLower);
}

/// For a layer `@A::@B::@C` in module Module,
/// the generated module is called `Module_A_B_C`.
static SmallString<32> moduleNameForLayer(StringRef moduleName,
SymbolRefAttr layerName) {
SmallString<32> result;
appendName(moduleName, result);
appendName(layerName, result);
return result;
}

/// For a layerblock `@A::@B::@C`,
/// the generated instance is called `a_b_c`.
static SmallString<32> instanceNameForLayer(SymbolRefAttr layerName) {
SmallString<32> result;
appendName(layerName, result, /*toLower=*/true);
return result;
}

/// For all layerblocks `@A::@B::@C` in a circuit called Circuit,
/// the output filename is `layers_Circuit_A_B_C.sv`.
static SmallString<32> fileNameForLayer(StringRef circuitName,
SymbolRefAttr layerName) {
SmallString<32> result;
result.append("layers");
appendName(circuitName, result);
appendName(layerName, result);
result.append(".sv");
return result;
}

//===----------------------------------------------------------------------===//
// LowerLayersPass
//===----------------------------------------------------------------------===//

class LowerLayersPass : public LowerLayersBase<LowerLayersPass> {
/// Safely build a new module with a given namehint. This handles geting a
/// lock to modify the top-level circuit.
Expand Down Expand Up @@ -234,14 +288,6 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
if (!layerBlock)
return WalkResult::advance();

// Compute the expanded layer name. For layer @A::@B::@C, this is "A_B_C".
SmallString<32> layerName(layerBlock.getLayerName().getRootReference());
for (auto ref : layerBlock.getLayerName().getNestedReferences()) {
layerName.append("_");
layerName.append(ref.getValue());
}
LLVM_DEBUG(llvm::dbgs() << " - Layer: " << layerName << "\n");

Block *body = layerBlock.getBody(0);
OpBuilder builder(moduleOp);

Expand All @@ -254,7 +300,7 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,

// Create an input port for an operand that is captured from outside.
auto createInputPort = [&](Value operand, Location loc) {
auto operandName = getFieldName(FieldRef(operand, 0), true);
const auto &[portName, _] = getFieldName(FieldRef(operand, 0), true);

// If the value is a ref, we must resolve the ref inside the parent,
// passing the input as a value instead of a ref. Inside the layer, we
Expand All @@ -263,8 +309,8 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
if (auto refType = dyn_cast<RefType>(type))
type = refType.getType();

ports.push_back({builder.getStringAttr("_" + operandName.first), type,
Direction::In, /*sym=*/{},
ports.push_back({builder.getStringAttr(portName), type, Direction::In,
/*sym=*/{},
/*loc=*/loc});
// Update the layer block's body with arguments as we will swap this body
// into the module when we create it. If this is a ref type, then add a
Expand Down Expand Up @@ -310,7 +356,7 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
auto createOutputPort = [&](Value dest, Value src) {
auto loc = getPortLoc(dest);
auto portNum = ports.size();
auto operandName = getFieldName(FieldRef(dest, 0), true);
const auto &[portName, _] = getFieldName(FieldRef(dest, 0), true);

RefType refType;
if (auto oldRef = dyn_cast<RefType>(dest.getType()))
Expand All @@ -320,8 +366,8 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
type_cast<FIRRTLBaseType>(dest.getType()).getPassiveType(),
/*forceable=*/false);

ports.push_back({builder.getStringAttr("_" + operandName.first), refType,
Direction::Out, /*sym=*/{}, /*loc=*/loc});
ports.push_back({builder.getStringAttr(portName), refType, Direction::Out,
/*sym=*/{}, /*loc=*/loc});
Value replacement = body->addArgument(refType, loc);
if (isa<RefType>(dest.getType())) {
dest.replaceUsesWithIf(replacement, [&](OpOperand &use) {
Expand Down Expand Up @@ -476,9 +522,7 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
// of the pass. This is done to avoid having to revisit and rewrite each
// instance everytime it is moved into a parent layer.
builder.setInsertionPointAfter(layerBlock);
auto moduleName = newModule.getModuleName();
auto instanceName =
(Twine((char)tolower(moduleName[0])) + moduleName.drop_front()).str();
auto instanceName = instanceNameForLayer(layerBlock.getLayerName());
auto instanceOp = builder.create<InstanceOp>(
layerBlock.getLoc(), /*moduleName=*/newModule,
/*name=*/
Expand All @@ -489,12 +533,11 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
(innerSyms.empty() ? hw::InnerSymAttr{}
: hw::InnerSymAttr::get(builder.getStringAttr(
ns.newName(instanceName)))));
instanceOp->setAttr("output_file",
hw::OutputFileAttr::getFromFilename(
builder.getContext(),

"layers_" + circuitName + "_" + layerName + ".sv",
/*excludeFromFileList=*/true));
auto fileName = fileNameForLayer(circuitName, layerBlock.getLayerName());
instanceOp->setAttr("output_file", hw::OutputFileAttr::getFromFilename(
builder.getContext(), fileName,
/*excludeFromFileList=*/true));
createdInstances.try_emplace(instanceOp, newModule);

LLVM_DEBUG(llvm::dbgs() << " moved inner refs:\n");
Expand Down Expand Up @@ -559,13 +602,9 @@ void LowerLayersPass::runOnOperation() {
CircuitNamespace ns(circuitOp);
circuitOp->walk([&](FModuleOp moduleOp) {
moduleOp->walk([&](LayerBlockOp layerBlockOp) {
SmallString<32> layerName(layerBlockOp.getLayerName().getRootReference());
for (auto ref : layerBlockOp.getLayerName().getNestedReferences()) {
layerName.append("_");
layerName.append(ref.getValue());
}
moduleNames.insert({layerBlockOp, ns.newName(moduleOp.getModuleName() +
"_" + layerName)});
auto name = moduleNameForLayer(moduleOp.getModuleName(),
layerBlockOp.getLayerName());
moduleNames.insert({layerBlockOp, ns.newName(name)});
});
});

Expand Down
10 changes: 5 additions & 5 deletions test/Dialect/FIRRTL/lower-layers.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ firrtl.circuit "Test" {

// Src Outside Layerblock.
//
// CHECK: firrtl.module private @[[A:.+]](in %_: !firrtl.uint<1>) {
// CHECK: %0 = firrtl.ref.send %_ : !firrtl.uint<1>
// CHECK: firrtl.module private @[[A:.+]](in %[[p:.+]]: !firrtl.uint<1>) {
// CHECK: %0 = firrtl.ref.send %[[p]] : !firrtl.uint<1>
// CHECK: %1 = firrtl.wire : !firrtl.probe<uint<1>>
// CHECK: firrtl.ref.define %1, %0 : !firrtl.probe<uint<1>>
// CHECK: }
Expand Down Expand Up @@ -422,19 +422,19 @@ firrtl.circuit "Simple" {
// CHECK: firrtl.module @Simple() {
// CHECK-NOT: firrtl.module
// CHECK-NOT: firrtl.layerblock
// CHECK: %[[A_B_C_cc:[_a-zA-Z0-9]+]] = firrtl.instance simple_A_B_C {
// CHECK: %[[A_B_C_cc:[_a-zA-Z0-9]+]] = firrtl.instance a_b_c {
// CHECK-SAME: lowerToBind
// CHECK-SAME: output_file = #hw.output_file<"layers_Simple_A_B_C.sv"
// CHECK-SAME: excludeFromFileList
// CHECK-SAME: @Simple_A_B_C(
// CHECK-NEXT: %[[A_B_b:[_a-zA-Z0-9]+]], %[[A_B_c:[_a-zA-Z0-9]+]], %[[A_B_cc:[_a-zA-Z0-9]+]] = firrtl.instance simple_A_B {
// CHECK-NEXT: %[[A_B_b:[_a-zA-Z0-9]+]], %[[A_B_c:[_a-zA-Z0-9]+]], %[[A_B_cc:[_a-zA-Z0-9]+]] = firrtl.instance a_b {
// CHECK-SAME: lowerToBind
// CHECK-SAME: output_file = #hw.output_file<"layers_Simple_A_B.sv", excludeFromFileList>
// CHECK-SAME: @Simple_A_B(
// CHECK-NEXT: %[[A_B_cc_resolve:[_a-zA-Z0-9]+]] = firrtl.ref.resolve %[[A_B_cc]]
// CHECK-NEXT: firrtl.strictconnect %[[A_B_C_cc]], %[[A_B_cc_resolve]]
// CHECK-NEXT: firrtl.strictconnect %[[A_B_b]], %b
// CHECK-NEXT: %[[A_a:[_a-zA-Z0-9]+]], %[[A_c:[_a-zA-Z0-9]+]] = firrtl.instance simple_A {
// CHECK-NEXT: %[[A_a:[_a-zA-Z0-9]+]], %[[A_c:[_a-zA-Z0-9]+]] = firrtl.instance a {
// CHECK-SAME: lowerToBind
// CHECK-SAME: output_file = #hw.output_file<"layers_Simple_A.sv", excludeFromFileList>
// CHECK-SAME: @Simple_A(
Expand Down
16 changes: 8 additions & 8 deletions test/firtool/layers.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,30 @@ circuit Foo: %[[
node y = x

; CHECK-LABEL: module Foo_A_B(
; CHECK-NEXT: input _x
; CHECK-NEXT: input x
; CHECK-NEXT: );
; CHECK: wire y = _x;
; CHECK: wire y = x;
; CHECK-NEXT: endmodule

; CHECK-LABEL: module Foo_A(
; CHECK-NEXT: input _in
; CHECK: wire x = _in;
; CHECK-NEXT: input in
; CHECK: wire x = in;
; CHECK-NEXT: wire x_probe = x;
; CHECK-NEXT: endmodule

; CHECK-LABEL: FILE "layers_Foo_A_B.sv"
; CHECK: `include "layers_Foo_A.sv"
; CHECK-NEXT: `ifndef layers_Foo_A_B
; CHECK-NEXT: `define layers_Foo_A_B
; CHECK-NEXT: bind Foo Foo_A_B foo_A_B (
; CHECK-NEXT: _x (Foo.foo_A.x_probe)
; CHECK-NEXT: bind Foo Foo_A_B a_b (
; CHECK-NEXT: x (Foo.a.x_probe)
; CHECK-NEXT: );
; CHECK-NEXT: `endif // layers_Foo_A_B

; CHECK-LABEL: FILE "layers_Foo_A.sv"
; CHECK: `ifndef layers_Foo_A
; CHECK-NEXT: `define layers_Foo_A
; CHECK-NEXT: bind Foo Foo_A foo_A (
; CHECK-NEXT: ._in (in)
; CHECK-NEXT: bind Foo Foo_A a (
; CHECK-NEXT: .in (in)
; CHECK-NEXT: );
; CHECK-NEXT: `endif // layers_Foo_A
3 changes: 1 addition & 2 deletions test/firtool/lower-layers.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ circuit Foo: %[[
define b = probe(c)

; CHECK: module Foo();
; CHECK: wire d = Foo.bar.bar_Verification.c_probe;
; CHECK: wire d = Foo.bar.verification.c_probe;
; CHECK: Bar bar ();
; CHECK: endmodule
public module Foo enablelayer Verification:
Expand All @@ -37,4 +37,3 @@ circuit Foo: %[[

node d = read(bar.b)
connect bar.a, d

34 changes: 17 additions & 17 deletions test/firtool/lower-layers2.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ circuit TestHarness:
layer Verification bind:

; CHECK: module DUT_Verification(
; CHECK: input _clock,
; CHECK: input [31:0] _a
; CHECK: input clock,
; CHECK: input [31:0] a
; CHECK: );
; CHECK: reg [31:0] pc_d;
; CHECK: wire [31:0] pc_d_probe = pc_d;
; CHECK: always @(posedge _clock)
; CHECK: pc_d <= _a;
; CHECK: always @(posedge clock)
; CHECK: pc_d <= a;
; CHECK: endmodule

; CHECK: module DUT(
Expand Down Expand Up @@ -52,14 +52,14 @@ circuit TestHarness:
define trace = x

; CHECK: module TestHarness_Verification(
; CHECK: input [31:0] _dut_trace,
; CHECK: input _clock,
; CHECK: _reset
; CHECK: input [31:0] dut_trace,
; CHECK: input clock,
; CHECK: reset
; CHECK: );
; CHECK: `ifndef SYNTHESIS
; CHECK: always @(posedge _clock) begin
; CHECK: if ((`PRINTF_COND_) & _reset)
; CHECK: $fwrite(32'h80000002, "The last PC was: %x", _dut_trace);
; CHECK: always @(posedge clock) begin
; CHECK: if ((`PRINTF_COND_) & reset)
; CHECK: $fwrite(32'h80000002, "The last PC was: %x", dut_trace);
; CHECK: end // always @(posedge)
; CHECK: `endif // not def SYNTHESIS
; CHECK: endmodule
Expand Down Expand Up @@ -94,13 +94,13 @@ circuit TestHarness:
; CHECK: // ----- 8< ----- FILE "layers_TestHarness_Verification.sv" ----- 8< -----
; CHECK: `ifndef layers_TestHarness_Verification
; CHECK: `define layers_TestHarness_Verification
; CHECK: bind DUT DUT_Verification dUT_Verification (
; CHECK: ._clock (clock),
; CHECK: ._a (a)
; CHECK: bind DUT DUT_Verification verification (
; CHECK: .clock (clock),
; CHECK: .a (a)
; CHECK: );
; CHECK: bind TestHarness TestHarness_Verification testHarness_Verification (
; CHECK: ._dut_trace (TestHarness.dut.dUT_Verification.pc_d_probe),
; CHECK: ._clock (clock),
; CHECK: ._reset (reset)
; CHECK: bind TestHarness TestHarness_Verification verification (
; CHECK: .dut_trace (TestHarness.dut.verification.pc_d_probe),
; CHECK: .clock (clock),
; CHECK: .reset (reset)
; CHECK: );
; CHECK: `endif // layers_TestHarness_Verification

0 comments on commit 6a768b2

Please sign in to comment.