From 081c3f678744d449289391e5e335558fe65cfcc4 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 6 Dec 2024 00:45:54 -0500 Subject: [PATCH] [FIRRTL] Align inline layer ABI w/ spec proposal Partially implement the inline layer convention ABI as described in the FIRRTL Spec/ABI PR for this feature [[1]]. This changes the name of the macro to be something that is more descriptive and cannot collide with FIRRTL identifiers (which are supposed to not include `$`). This does not implement the full ABI. Namely, it does not correctly handle circuits with multiple public modules. (Notably, this isn't implemented for the bind layer convention either.) This will be fixed in a follow-on. [1]: https://github.com/chipsalliance/firrtl-spec/pull/262 Signed-off-by: Schuyler Eldridge --- lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp | 25 +++++++++++-------- test/Dialect/FIRRTL/lower-layers.mlir | 12 ++++----- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp index 7ef527d4cc35..c2324a9c04a6 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp @@ -121,8 +121,10 @@ static SmallString<32> fileNameForLayer(StringRef circuitName, /// For a layerblock `@A::@B::@C`, the verilog macro is `A_B_C`. static SmallString<32> -macroNameForLayer(ArrayRef layerName) { - SmallString<32> result; +macroNameForLayer(StringRef circuitName, + ArrayRef layerName) { + SmallString<32> result("layer_"); + result.append(circuitName); for (auto part : layerName) appendName(part, result, /*toLower=*/false, /*delimiter=*/Delimiter::InlineMacro); @@ -182,8 +184,10 @@ class LowerLayersPass /// Preprocess layers to build macro declarations and cache information about /// the layers so that this can be quired later. void preprocessLayers(CircuitNamespace &ns, OpBuilder &b, LayerOp layer, + StringRef circuitName, SmallVector &stack); - void preprocessLayers(CircuitNamespace &ns, LayerOp layer); + void preprocessLayers(CircuitNamespace &ns, LayerOp layer, + StringRef circuitName); /// Entry point for the function. void runOnOperation() override; @@ -731,7 +735,7 @@ LogicalResult LowerLayersPass::runOnModuleBody(FModuleOp moduleOp, } void LowerLayersPass::preprocessLayers(CircuitNamespace &ns, OpBuilder &b, - LayerOp layer, + LayerOp layer, StringRef circuitName, SmallVector &stack) { stack.emplace_back(FlatSymbolRefAttr::get(layer.getSymNameAttr())); ArrayRef stackRef(stack); @@ -740,7 +744,7 @@ void LowerLayersPass::preprocessLayers(CircuitNamespace &ns, OpBuilder &b, layer}); if (layer.getConvention() == LayerConvention::Inline) { auto *ctx = &getContext(); - auto macName = macroNameForLayer(stack); + auto macName = macroNameForLayer(circuitName, stack); auto symName = ns.newName(macName); auto symNameAttr = StringAttr::get(ctx, symName); @@ -753,14 +757,15 @@ void LowerLayersPass::preprocessLayers(CircuitNamespace &ns, OpBuilder &b, macroNames[layer] = FlatSymbolRefAttr::get(&getContext(), symNameAttr); } for (auto child : layer.getOps()) - preprocessLayers(ns, b, child, stack); + preprocessLayers(ns, b, child, circuitName, stack); stack.pop_back(); } -void LowerLayersPass::preprocessLayers(CircuitNamespace &ns, LayerOp layer) { +void LowerLayersPass::preprocessLayers(CircuitNamespace &ns, LayerOp layer, + StringRef circuitName) { OpBuilder b(layer); SmallVector stack; - preprocessLayers(ns, b, layer, stack); + preprocessLayers(ns, b, layer, circuitName, stack); } /// Process a circuit to remove all layer blocks in each module and top-level @@ -770,6 +775,7 @@ void LowerLayersPass::runOnOperation() { llvm::dbgs() << "==----- Running LowerLayers " "-------------------------------------------------===\n"); CircuitOp circuitOp = getOperation(); + StringRef circuitName = circuitOp.getName(); // Initialize members which cannot be initialized automatically. llvm::sys::SmartMutex mutex; @@ -790,7 +796,7 @@ void LowerLayersPass::runOnOperation() { // Build verilog macro declarations for each inline layer declarations. // Cache layer symbol refs for lookup later. if (auto layerOp = dyn_cast(op)) { - preprocessLayers(ns, layerOp); + preprocessLayers(ns, layerOp, circuitName); continue; } } @@ -865,7 +871,6 @@ void LowerLayersPass::runOnOperation() { // TODO: This would be better handled without the use of verbatim ops. OpBuilder builder(circuitOp); SmallVector> layers; - StringRef circuitName = circuitOp.getName(); circuitOp.walk([&](LayerOp layerOp) { auto parentOp = layerOp->getParentOfType(); while (!layers.empty() && parentOp != layers.back().first) diff --git a/test/Dialect/FIRRTL/lower-layers.mlir b/test/Dialect/FIRRTL/lower-layers.mlir index 2e7f35d3683a..ad5deb1fb9cb 100644 --- a/test/Dialect/FIRRTL/lower-layers.mlir +++ b/test/Dialect/FIRRTL/lower-layers.mlir @@ -396,9 +396,9 @@ firrtl.circuit "Test" { // Inline Layers //===--------------------------------------------------------------------===// - // CHECK: sv.macro.decl @[[INLINE:.*]]["Inline"] - // CHECK-NEXT: sv.macro.decl @Inline$Inline - // CHECK-NEXT: sv.macro.decl @Bound$Inline + // CHECK: sv.macro.decl @layer_Test$Inline + // CHECK-NEXT: sv.macro.decl @layer_Test$Inline$Inline + // CHECK-NEXT: sv.macro.decl @layer_Test$Bound$Inline firrtl.layer @Inline inline { firrtl.layer @Inline inline {} } @@ -409,15 +409,15 @@ firrtl.circuit "Test" { // CHECK: firrtl.module private @ModuleWithInlineLayerBlocks_Bound() { // CHECK-NEXT: %w3 = firrtl.wire - // CHECK-NEXT: sv.ifdef @Bound$Inline { + // CHECK-NEXT: sv.ifdef @layer_Test$Bound$Inline { // CHECK-NEXT: %w4 = firrtl.wire // CHECK-NEXT: } // CHECK-NEXT: } // CHECK-NEXT: firrtl.module @ModuleWithInlineLayerBlocks() { - // CHECK-NEXT: sv.ifdef @[[INLINE]] { + // CHECK-NEXT: sv.ifdef @layer_Test$Inline { // CHECK-NEXT: %w1 = firrtl.wire - // CHECK-NEXT: sv.ifdef @Inline$Inline { + // CHECK-NEXT: sv.ifdef @layer_Test$Inline$Inline { // CHECK-NEXT: %w2 = firrtl.wire // CHECK-NEXT: } // CHECK-NEXT: }