Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIRRTL][LowerLayers] Clean up names of artifacts generated by layers #6733

Merged
merged 1 commit into from
Feb 26, 2024
Merged

[FIRRTL][LowerLayers] Clean up names of artifacts generated by layers #6733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
105 changes: 72 additions & 33 deletions lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp
View file
Open in desktop
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();
dtzSiFive marked this conversation as resolved.
Show resolved Hide resolved
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) {
rwy7 marked this conversation as resolved.
Show resolved Hide resolved
SmallString<32> result;
appendName(moduleName, result);
appendName(layerName, result);
return result;
rwy7 marked this conversation as resolved.
Show resolved Hide resolved
}

/// 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);
rwy7 marked this conversation as resolved.
Show resolved Hide resolved
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change for FIRRTL 4.0?

abi.md presently reads:

filename = "layers_" , module , "_", root , { "_" , nested } , ".sv" ;

(module not circuit)

Is this going to work for public modules that aren't the main module as-is?

Copy link
Member

Choose a reason for hiding this comment

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

I think this pass does not do the right thing for public modules. This happens to work now for the main module. The pass should be updated to emit the collateral for all public modules in a follow-on. Good catch, Will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an issue: #6748

SymbolRefAttr layerName) {
SmallString<32> result;
result.append("layers");
appendName(circuitName, result);
appendName(layerName, result);
result.append(".sv");
return result;
}
seldridge marked this conversation as resolved.
Show resolved Hide resolved

//===----------------------------------------------------------------------===//
// 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);
Comment on lines -257 to +303
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup.


// 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());
Comment on lines -479 to +525
Copy link
Member

Choose a reason for hiding this comment

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

This is so much cleaner. 💯

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading