From a645ea638a2716db1fb2296cab1a6d4b1ce0e031 Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Wed, 24 Jul 2024 10:24:24 -0500 Subject: [PATCH 1/4] [FIRRTL] Inliner: Support for ops with regions. inlineInstances/flattenInstances: * Walk entire body, not only top-level operations. Fixes missing instances and allows inlining them when conservatively legal. * Reject inlining instances under when/match. inlineInto/flattenInto: Walk entire body using new `inliningWalk` method that drives the per-operations handling but also handles cloning "structure" operations that have regions (when/match/layer) and managing what should be cloned where. This allows inlining modules that contain these operations. Inliner now may produce errors, thread throughout. This allows the inliner to run earlier in the pipeline, particularly before LowerLayers. --- .../FIRRTL/Transforms/ModuleInliner.cpp | 289 +++++++++++++----- test/Dialect/FIRRTL/inliner-errors.mlir | 91 ++++++ test/Dialect/FIRRTL/inliner.mlir | 115 +++++++ 3 files changed, 420 insertions(+), 75 deletions(-) create mode 100644 test/Dialect/FIRRTL/inliner-errors.mlir diff --git a/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp b/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp index 6e3ef50a16b9..10db6d538b7e 100644 --- a/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp +++ b/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp @@ -497,7 +497,7 @@ class Inliner { Inliner(CircuitOp circuit, SymbolTable &symbolTable); /// Run the inliner. - void run(); + LogicalResult run(); private: /// Inlining context, one per module being inlined into. @@ -574,23 +574,36 @@ class Inliner { /// Returns true if the operation is annotated to be inlined. bool shouldInline(Operation *op); + /// Check not inlining into anything other than layerblock or module. + /// In the future, could check this per-inlined-operation. + LogicalResult checkInstanceParents(InstanceOp instance); + + /// Walk the specified block, invoking `process` for operations visited + /// forward+pre-order. Handles cloning supported operations with regions, + /// so that `process` is only invoked on regionless operations. + LogicalResult + inliningWalk(OpBuilder &builder, Block *block, IRMapping &mapper, + llvm::function_ref process); + /// Flattens a target module into the insertion point of the builder, /// renaming all operations using the prefix. This clones all operations from /// the target, and does not trigger inlining on the target itself. - void flattenInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, - DenseSet localSymbols); + LogicalResult flattenInto(StringRef prefix, InliningLevel &il, + IRMapping &mapper, + DenseSet localSymbols); /// Inlines a target module into the insertion point of the builder, /// prefixing all operations with prefix. This clones all operations from /// the target, and does not trigger inlining on the target itself. - void inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, - DenseMap &symbolRenames); + LogicalResult inlineInto(StringRef prefix, InliningLevel &il, + IRMapping &mapper, + DenseMap &symbolRenames); /// Recursively flatten all instances in a module. - void flattenInstances(FModuleOp module); + LogicalResult flattenInstances(FModuleOp module); /// Inline any instances in the module which were marked for inlining. - void inlineInstances(FModuleOp module); + LogicalResult inlineInstances(FModuleOp module); /// Create a debug scope for an inlined instance at the current insertion /// point of the `il.mic` builder. @@ -897,26 +910,19 @@ void Inliner::cloneAndRename( } // Clone and rename. - auto *newOp = il.mic.b.clone(op, mapper); + assert(op.getNumRegions() == 0 && + "operation with regions should not reach cloneAndRename"); + auto *newOp = il.mic.b.cloneWithoutRegions(op, mapper); - // Rename the new operation and any contained operations. + // Rename the new operation. // (add prefix to it, if named, and unique-ify symbol, updating NLA's). - op.walk([&](Operation *origOp) { - auto *newOpToRename = mapper.lookup(origOp); - assert(newOpToRename); - // TODO: If want to work before ExpandWhen's, more work needed! - // Handle what we can for now. - assert((origOp == &op || !isa(origOp)) && - "Cannot handle instances not at top-level"); - - // Instances require extra handling to update HierPathOp's if their symbols - // change. - if (auto oldInst = dyn_cast(origOp)) - renameInstance(prefix, il, oldInst, cast(newOpToRename), - symbolRenames); - else - rename(prefix, newOpToRename, il); - }); + + // Instances require extra handling to update HierPathOp's if their symbols + // change. + if (auto oldInst = dyn_cast(op)) + renameInstance(prefix, il, oldInst, cast(newOp), symbolRenames); + else + rename(prefix, newOp, il); // We want to avoid attaching an empty annotation array on to an op that // never had an annotation array in the first place. @@ -934,18 +940,122 @@ bool Inliner::shouldInline(Operation *op) { return AnnotationSet::hasAnnotation(op, inlineAnnoClass); } +LogicalResult Inliner::inliningWalk( + OpBuilder &builder, Block *block, IRMapping &mapper, + llvm::function_ref process) { + struct IPs { + OpBuilder::InsertPoint target; + Block::iterator source; + }; + // Invariant: no Block::iterator == end(), can't getBlock(). + SmallVector inliningStack; + if (block->empty()) + return success(); + + inliningStack.push_back(IPs{builder.saveInsertionPoint(), block->begin()}); + OpBuilder::InsertionGuard guard(builder); + while (!inliningStack.empty()) { + auto &ips = inliningStack.back(); + builder.restoreInsertionPoint(ips.target); + auto end = ips.source->getBlock()->end(); + for (; ips.source != end; ++ips.source) { + auto &source = *ips.source; + + // Handle ops with regions below. + if (source.getNumRegions() != 0) + break; + + assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); + // Clone source into insertion point 'target'. + if (failed(process(&source))) + return failure(); + assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); + } + + // If we've finished inlining this block, pop and continue to next. + if (ips.source == end) { + inliningStack.pop_back(); + continue; + } + + // Otherwise, we have an operation with regions. + auto &sourceWithRegions = *ips.source; + assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); + + // Advance source iterator, we'll handle this operation below. + // If this is the last operation of current level, pop off stack. + if (++ips.source == end) + inliningStack.pop_back(); + + // Limited support for region-containing operations. + if (!isa(sourceWithRegions)) + return sourceWithRegions.emitError("unsupported operation '") + << sourceWithRegions.getName() << "' cannot be inlined"; + + // Note: This does not use cloneAndRename for simplicity, + // as there are no annotations, symbols to rename, or names + // to prefix. This does mean these operations do not appear + // in `il.newOps` for inner-ref renaming walk, FWIW. + auto *newOp = builder.cloneWithoutRegions(sourceWithRegions, mapper); + for (auto [newRegion, oldRegion] : llvm::reverse(llvm::zip_equal( + newOp->getRegions(), sourceWithRegions.getRegions()))) { + // If region has no blocks, skip. + if (oldRegion.empty()) { + assert(newRegion.empty()); + continue; + } + // Otherwise, assert single block. Multiple blocks is trickier. + assert(oldRegion.hasOneBlock()); + + // Create new block and add to inlining stack for processing. + auto &oldBlock = oldRegion.getBlocks().front(); + auto &newBlock = newRegion.emplaceBlock(); + mapper.map(&oldBlock, &newBlock); + + // Copy block arguments, and add mapping for each. + for (auto arg : oldBlock.getArguments()) + mapper.map(arg, newBlock.addArgument(arg.getType(), arg.getLoc())); + + if (oldBlock.empty()) + continue; + + inliningStack.push_back( + IPs{OpBuilder::InsertPoint(&newBlock, newBlock.begin()), + oldBlock.begin()}); + } + } + return success(); +} + +LogicalResult Inliner::checkInstanceParents(InstanceOp instance) { + auto *parent = instance->getParentOp(); + while (!isa(parent)) { + if (!isa(parent)) + return instance->emitError("cannot inline instance") + .attachNote(parent->getLoc()) + << "containing operation '" << parent->getName() + << "' not safe to inline into"; + parent = parent->getParentOp(); + } + return success(); +} + // NOLINTNEXTLINE(misc-no-recursion) -void Inliner::flattenInto(StringRef prefix, InliningLevel &il, - IRMapping &mapper, DenseSet localSymbols) { +LogicalResult Inliner::flattenInto(StringRef prefix, InliningLevel &il, + IRMapping &mapper, + DenseSet localSymbols) { auto target = il.childModule; auto moduleName = target.getNameAttr(); DenseMap symbolRenames; - for (auto &op : *target.getBodyBlock()) { + + LLVM_DEBUG(llvm::dbgs() << "flattening " << target.getModuleName() << " into " + << il.mic.module.getModuleName() << "\n"); + auto visit = [&](Operation *op) { // If it's not an instance op, clone it and continue. auto instance = dyn_cast(op); if (!instance) { - cloneAndRename(prefix, il, mapper, op, symbolRenames, localSymbols); - continue; + cloneAndRename(prefix, il, mapper, *op, symbolRenames, localSymbols); + return success(); } // If it's not a regular module we can't inline it. Mark it as live. @@ -954,10 +1064,13 @@ void Inliner::flattenInto(StringRef prefix, InliningLevel &il, if (!childModule) { liveModules.insert(moduleOp); - cloneAndRename(prefix, il, mapper, op, symbolRenames, localSymbols); - continue; + cloneAndRename(prefix, il, mapper, *op, symbolRenames, localSymbols); + return success(); } + if (failed(checkInstanceParents(instance))) + return failure(); + // Add any NLAs which start at this instance to the localSymbols set. // Anything in this set will be made local during the recursive flattenInto // walk. @@ -976,29 +1089,31 @@ void Inliner::flattenInto(StringRef prefix, InliningLevel &il, mapResultsToWires(mapper, childIL.wires, instance); // Unconditionally flatten all instance operations. - flattenInto(nestedPrefix, childIL, mapper, localSymbols); + if (failed(flattenInto(nestedPrefix, childIL, mapper, localSymbols))) + return failure(); currentPath.pop_back(); activeHierpaths = parentActivePaths; - } + return success(); + }; + return inliningWalk(il.mic.b, target.getBodyBlock(), mapper, visit); } -void Inliner::flattenInstances(FModuleOp module) { +LogicalResult Inliner::flattenInstances(FModuleOp module) { auto moduleName = module.getNameAttr(); ModuleInliningContext mic(module); - for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) { - // If it's not an instance op, skip it. - auto instance = dyn_cast(op); - if (!instance) - continue; - + auto visit = [&](InstanceOp instance) { // If it's not a regular module we can't inline it. Mark it as live. auto *targetModule = symbolTable.lookup(instance.getModuleName()); auto target = dyn_cast(targetModule); if (!target) { liveModules.insert(targetModule); - continue; + return WalkResult::advance(); } + + if (failed(checkInstanceParents(instance))) + return WalkResult::interrupt(); + if (auto instSym = getInnerSymName(instance)) { auto innerRef = InnerRefAttr::get(moduleName, instSym); // Preorder update of any non-local annotations this instance participates @@ -1032,28 +1147,37 @@ void Inliner::flattenInstances(FModuleOp module) { instance.getResult(i).replaceAllUsesWith(il.wires[i]); // Recursively flatten the target module. - flattenInto(nestedPrefix, il, mapper, localSymbols); + if (failed(flattenInto(nestedPrefix, il, mapper, localSymbols))) + return WalkResult::interrupt(); currentPath.pop_back(); activeHierpaths = parentActivePaths; // Erase the replaced instance. instance.erase(); - } + return WalkResult::skip(); + }; + return failure(module.getBodyBlock() + ->walk(visit) + .wasInterrupted()); } // NOLINTNEXTLINE(misc-no-recursion) -void Inliner::inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, - DenseMap &symbolRenames) { +LogicalResult +Inliner::inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, + DenseMap &symbolRenames) { auto target = il.childModule; auto inlineToParent = il.mic.module; auto moduleName = target.getNameAttr(); - // Inline everything in the module's body. - for (auto &op : *target.getBodyBlock()) { + + LLVM_DEBUG(llvm::dbgs() << "inlining " << target.getModuleName() << " into " + << inlineToParent.getModuleName() << "\n"); + + auto visit = [&](Operation *op) { // If it's not an instance op, clone it and continue. auto instance = dyn_cast(op); if (!instance) { - cloneAndRename(prefix, il, mapper, op, symbolRenames, {}); - continue; + cloneAndRename(prefix, il, mapper, *op, symbolRenames, {}); + return success(); } // If it's not a regular module we can't inline it. Mark it as live. @@ -1061,8 +1185,8 @@ void Inliner::inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, auto childModule = dyn_cast(moduleOp); if (!childModule) { liveModules.insert(moduleOp); - cloneAndRename(prefix, il, mapper, op, symbolRenames, {}); - continue; + cloneAndRename(prefix, il, mapper, *op, symbolRenames, {}); + return success(); } // If we aren't inlining the target, add it to the work list. @@ -1070,10 +1194,13 @@ void Inliner::inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, if (liveModules.insert(childModule).second) { worklist.push_back(childModule); } - cloneAndRename(prefix, il, mapper, op, symbolRenames, {}); - continue; + cloneAndRename(prefix, il, mapper, *op, symbolRenames, {}); + return success(); } + if (failed(checkInstanceParents(instance))) + return failure(); + auto toBeFlattened = shouldFlatten(childModule); if (auto instSym = getInnerSymName(instance)) { auto innerRef = InnerRefAttr::get(moduleName, instSym); @@ -1132,34 +1259,32 @@ void Inliner::inlineInto(StringRef prefix, InliningLevel &il, IRMapping &mapper, // Inline the module, it can be marked as flatten and inline. if (toBeFlattened) { - flattenInto(nestedPrefix, childIL, mapper, {}); + if (failed(flattenInto(nestedPrefix, childIL, mapper, {}))) + return failure(); } else { - inlineInto(nestedPrefix, childIL, mapper, symbolRenames); + if (failed(inlineInto(nestedPrefix, childIL, mapper, symbolRenames))) + return failure(); } currentPath.pop_back(); activeHierpaths = parentActivePaths; - } + return success(); + }; + + return inliningWalk(il.mic.b, target.getBodyBlock(), mapper, visit); } -void Inliner::inlineInstances(FModuleOp module) { +LogicalResult Inliner::inlineInstances(FModuleOp module) { // Generate a namespace for this module so that we can safely inline symbols. auto moduleName = module.getNameAttr(); - - SmallVector wires; ModuleInliningContext mic(module); - for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) { - // If it's not an instance op, skip it. - auto instance = dyn_cast(op); - if (!instance) - continue; - + auto visit = [&](InstanceOp instance) { // If it's not a regular module we can't inline it. Mark it as live. auto *childModule = symbolTable.lookup(instance.getModuleName()); auto target = dyn_cast(childModule); if (!target) { liveModules.insert(childModule); - continue; + return WalkResult::advance(); } // If we aren't inlining the target, add it to the work list. @@ -1167,9 +1292,12 @@ void Inliner::inlineInstances(FModuleOp module) { if (liveModules.insert(target).second) { worklist.push_back(target); } - continue; + return WalkResult::advance(); } + if (failed(checkInstanceParents(instance))) + return WalkResult::interrupt(); + auto toBeFlattened = shouldFlatten(target); if (auto instSym = getInnerSymName(instance)) { auto innerRef = InnerRefAttr::get(moduleName, instSym); @@ -1224,18 +1352,25 @@ void Inliner::inlineInstances(FModuleOp module) { // Inline the module, it can be marked as flatten and inline. if (toBeFlattened) { - flattenInto(nestedPrefix, childIL, mapper, {}); + if (failed(flattenInto(nestedPrefix, childIL, mapper, {}))) + return WalkResult::interrupt(); } else { // Recursively inline all the child modules under `parent`, that are // marked to be inlined. - inlineInto(nestedPrefix, childIL, mapper, symbolRenames); + if (failed(inlineInto(nestedPrefix, childIL, mapper, symbolRenames))) + return WalkResult::interrupt(); } currentPath.pop_back(); activeHierpaths = parentActivePaths; // Erase the replaced instance. instance.erase(); - } + return WalkResult::skip(); + }; + + return failure(module.getBodyBlock() + ->walk(visit) + .wasInterrupted()); } void Inliner::createDebugScope(InliningLevel &il, InstanceOp instance, @@ -1316,7 +1451,7 @@ Inliner::Inliner(CircuitOp circuit, SymbolTable &symbolTable) : circuit(circuit), context(circuit.getContext()), symbolTable(symbolTable) {} -void Inliner::run() { +LogicalResult Inliner::run() { CircuitNamespace circuitNamespace(circuit); // Gather all NLA's, build information about the instance ops used: @@ -1346,13 +1481,15 @@ void Inliner::run() { while (!worklist.empty()) { auto moduleOp = worklist.pop_back_val(); if (shouldFlatten(moduleOp)) { - flattenInstances(moduleOp); + if (failed(flattenInstances(moduleOp))) + return failure(); // Delete the flatten annotation, the transform was performed. // Even if visited again in our walk (for inlining), // we've just flattened it and so the annotation is no longer needed. AnnotationSet::removeAnnotations(moduleOp, flattenAnnoClass); } else { - inlineInstances(moduleOp); + if (failed(inlineInstances(moduleOp))) + return failure(); } } @@ -1468,6 +1605,7 @@ void Inliner::run() { fmodule->setAttr("portAnnotations", ArrayAttr::get(context, newPortAnnotations)); } + return success(); } //===----------------------------------------------------------------------===// @@ -1479,7 +1617,8 @@ class InlinerPass : public circt::firrtl::impl::InlinerBase { void runOnOperation() override { LLVM_DEBUG(debugPassHeader(this) << "\n"); Inliner inliner(getOperation(), getAnalysis()); - inliner.run(); + if (failed(inliner.run())) + signalPassFailure(); LLVM_DEBUG(debugFooter() << "\n"); } }; diff --git a/test/Dialect/FIRRTL/inliner-errors.mlir b/test/Dialect/FIRRTL/inliner-errors.mlir new file mode 100644 index 000000000000..c4a557f19e47 --- /dev/null +++ b/test/Dialect/FIRRTL/inliner-errors.mlir @@ -0,0 +1,91 @@ +// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl-inliner))' -allow-unregistered-dialect -verify-diagnostics --split-input-file %s + +// Reject inlining into when (run ExpandWhens first). + +firrtl.circuit "InlineIntoWhen" { + firrtl.module private @Child () attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} {} + firrtl.module @InlineIntoWhen(in %cond : !firrtl.uint<1>) { + // expected-note @below {{containing operation 'firrtl.when' not safe to inline into}} + firrtl.when %cond : !firrtl.uint<1> { + // expected-error @below {{cannot inline instance}} + firrtl.instance c @Child() + } + } +} + +// ----- + +// Reject flattening through when (run ExpandWhens first). + +firrtl.circuit "FlattenThroughWhen" { + firrtl.module private @GChild () {} + firrtl.module private @Child (in %cond : !firrtl.uint<1>) { + // expected-note @below {{containing operation 'firrtl.when' not safe to inline into}} + firrtl.when %cond : !firrtl.uint<1> { + // expected-error @below {{cannot inline instance}} + firrtl.instance c @GChild() + } + } + firrtl.module @FlattenThroughWhen(in %cond : !firrtl.uint<1>) attributes {annotations = [{class = "firrtl.transforms.FlattenAnnotation"}]} { + %c_cond = firrtl.instance c @Child(in cond : !firrtl.uint<1>) + firrtl.matchingconnect %c_cond, %cond : !firrtl.uint<1> + } +} + +// ----- + +// Reject inlining into unrecognized operations. + +firrtl.circuit "InlineIntoIfdef" { + sv.macro.decl @A_0["A"] + firrtl.module private @Child () attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} {} + firrtl.module @InlineIntoIfdef() { + // expected-note @below {{containing operation 'sv.ifdef' not safe to inline into}} + sv.ifdef @A_0 { + // expected-error @below {{cannot inline instance}} + firrtl.instance c @Child() + } + } +} + +// ----- + +// Conservatively reject cloning operations with regions that we don't recognize. + +firrtl.circuit "InlineIfdef" { + sv.macro.decl @A_0["A"] + firrtl.module private @Child () attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + // expected-error @below {{unsupported operation 'sv.ifdef' cannot be inlined}} + sv.ifdef @A_0 { } + } + firrtl.module @InlineIfdef() { + firrtl.instance c @Child() + } +} + +// ----- + +// Cannot inline layers into layers. +// Presently the issue is detected by the verifier. + +firrtl.circuit "InlineLayerIntoLayer" { + firrtl.layer @I inline { + firrtl.layer @J inline { + } + } + firrtl.module private @MatchAgain(in %i: !firrtl.uint<8>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + // expected-error @below {{op has an un-nested layer symbol, but does not have a 'firrtl.module' op as a parent}} + firrtl.layerblock @I { + firrtl.layerblock @I::@J { + %n = firrtl.node interesting_name %i : !firrtl.uint<8> + } + } + } + firrtl.module @InlineLayerIntoLayer(in %i: !firrtl.uint<8>) attributes {convention = #firrtl} { + // expected-note @below {{illegal parent op defined here}} + firrtl.layerblock @I { + %c_i = firrtl.instance c interesting_name @MatchAgain(in i: !firrtl.uint<8>) + firrtl.matchingconnect %c_i, %i : !firrtl.uint<8> + } + } +} diff --git a/test/Dialect/FIRRTL/inliner.mlir b/test/Dialect/FIRRTL/inliner.mlir index e263cf6d02c8..e2aa3796d97d 100644 --- a/test/Dialect/FIRRTL/inliner.mlir +++ b/test/Dialect/FIRRTL/inliner.mlir @@ -1267,3 +1267,118 @@ firrtl.module private @test3() { %test_wire = firrtl.wire : !firrtl.uint<2> } } + +// ----- + +// Test inlining into nested layer, and cloning operations with blocks + blockargs (match). + +firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} { + firrtl.layer @I inline { + firrtl.layer @J inline { } + } + // CHECK-NOT: @GChild + firrtl.module private @GChild(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + %0 = firrtl.not %in : (!firrtl.uint<8>) -> !firrtl.uint<8> + firrtl.matchingconnect %out, %0 : !firrtl.uint<8> + } + firrtl.module private @Child(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) { + %gc_in, %gc_out = firrtl.instance gc @GChild(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) + firrtl.matchingconnect %gc_in, %in : !firrtl.uint<8> + firrtl.matchingconnect %out, %gc_out : !firrtl.uint<8> + } + // CHECK-NOT: @MatchAgain + firrtl.module private @MatchAgain(in %i: !firrtl.enum, None: uint<0>>, out %o: !firrtl.uint<8>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + firrtl.match %i : !firrtl.enum, None: uint<0>> { + case Some(%arg0) { + %c_in, %c_out = firrtl.instance c @Child(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) + firrtl.matchingconnect %c_in, %arg0 : !firrtl.uint<8> + firrtl.matchingconnect %o, %c_out : !firrtl.uint<8> + } + case None(%arg0) { + %c_in, %c_out = firrtl.instance c @Child(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) + %invalid_ui8 = firrtl.invalidvalue : !firrtl.uint<8> + firrtl.matchingconnect %c_in, %invalid_ui8 : !firrtl.uint<8> + firrtl.matchingconnect %o, %c_out : !firrtl.uint<8> + } + } + } + // CHECK: @MatchInline + firrtl.module @MatchInline(in %i: !firrtl.enum, None: uint<0>>, + out %o: !firrtl.probe, @I::@J>) attributes {convention = #firrtl} { + // CHECK-NEXT: layerblock @I + firrtl.layerblock @I { + // CHECK-NEXT: layerblock @I::@J + firrtl.layerblock @I::@J { + // CHECK-NOT: @MatchAgain + // CHECK: firrtl.match + // CHECK-NEXT: Some(%arg0) + // CHECK-NEXT: %c_c_in, %c_c_out = firrtl.instance c_c @Child + // CHECK-NEXT: firrtl.matchingconnect %c_c_in, %arg0 + // CHECK: None(%arg0) + // CHECK-NEXT: %c_c_in, %c_c_out = firrtl.instance c_c @Child + %c_i, %c_o = firrtl.instance c @MatchAgain(in i: !firrtl.enum, None: uint<0>>, out o: !firrtl.uint<8>) + firrtl.matchingconnect %c_i, %i : !firrtl.enum, None: uint<0>> + %0 = firrtl.ref.send %c_o : !firrtl.uint<8> + %1 = firrtl.ref.cast %0 : (!firrtl.probe>) -> !firrtl.probe, @I::@J> + firrtl.ref.define %o, %1 : !firrtl.probe, @I::@J> + } + } + } +} + +// ----- + +// Test inlining module containing various operations with blocks. + +firrtl.circuit "InlineBlocks" { + firrtl.layer @I inline { + firrtl.layer @J inline { } + } + firrtl.module private @HasBlocks(in %i: !firrtl.enum, None: uint<0>>, + in %cond: !firrtl.uint<1>, + out %p: !firrtl.probe, @I::@J>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + firrtl.layerblock @I { + firrtl.when %cond : !firrtl.uint<1> { + firrtl.layerblock @I::@J { + %o = firrtl.wire interesting_name : !firrtl.uint<8> + %0 = firrtl.ref.send %o : !firrtl.uint<8> + %1 = firrtl.ref.cast %0 : (!firrtl.probe>) -> !firrtl.probe, @I::@J> + firrtl.ref.define %p, %1 : !firrtl.probe, @I::@J> + firrtl.match %i : !firrtl.enum, None: uint<0>> { + case Some(%arg0) { + firrtl.matchingconnect %o, %arg0 : !firrtl.uint<8> + } + case None(%arg0) { + %invalid_ui8 = firrtl.invalidvalue : !firrtl.uint<8> + firrtl.matchingconnect %o, %invalid_ui8 : !firrtl.uint<8> + } + } + %unused = firrtl.node %o : !firrtl.uint<8> + } + %unused = firrtl.node %cond : !firrtl.uint<1> + } + } + } + // CHECK: @InlineBlocks + firrtl.module @InlineBlocks(in %i: !firrtl.enum, None: uint<0>>, in %cond: !firrtl.uint<1>, out %o: !firrtl.probe, @I::@J>) attributes {convention = #firrtl} { + // Check inlined structure. + // CHECK: layerblock @I + // CHECK-NEXT: firrtl.when + // CHECK-NEXT: firrtl.layerblock @I::@J + // CHECK-NEXT: firrtl.wire + // CHECK: firrtl.match + // CHECK: Some( + // CHECK: None( + // CHECK: } + // CHECK-NEXT: } + // CHECK-NEXT: firrtl.node {{.*}} + // CHECK-NEXT: } + // CHECK-NEXT: firrtl.node {{.*}} + // CHECK-NEXT: } + // CHECK-NEXT: } + %c_i, %c_cond, %c_p = firrtl.instance c interesting_name @HasBlocks(in i: !firrtl.enum, None: uint<0>>, in cond: !firrtl.uint<1>, out p: !firrtl.probe, @I::@J>) + firrtl.matchingconnect %c_i, %i : !firrtl.enum, None: uint<0>> + firrtl.matchingconnect %c_cond, %cond : !firrtl.uint<1> + firrtl.ref.define %o, %c_p : !firrtl.probe, @I::@J> + } +} From d362cb30c7f002f9744a2bb3e650287be11ba067 Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Thu, 26 Sep 2024 10:32:08 -0500 Subject: [PATCH 2/4] [FIRRTL][Inliner] rework inliningWalk a bit for easier reading. Does a bit more redundant work than needed in the tight loop re:cloning individual (regionless) operations, but avoids some tricky flow re:break and re-checking termination condition. --- .../FIRRTL/Transforms/ModuleInliner.cpp | 54 ++++++++----------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp b/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp index 10db6d538b7e..b19c8d6e936d 100644 --- a/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp +++ b/lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp @@ -954,51 +954,43 @@ LogicalResult Inliner::inliningWalk( inliningStack.push_back(IPs{builder.saveInsertionPoint(), block->begin()}); OpBuilder::InsertionGuard guard(builder); - while (!inliningStack.empty()) { - auto &ips = inliningStack.back(); - builder.restoreInsertionPoint(ips.target); - auto end = ips.source->getBlock()->end(); - for (; ips.source != end; ++ips.source) { - auto &source = *ips.source; - // Handle ops with regions below. - if (source.getNumRegions() != 0) - break; + while (!inliningStack.empty()) { + auto target = inliningStack.back().target; + builder.restoreInsertionPoint(target); + Operation *source; + // Get next source operation. + { + auto &ips = inliningStack.back(); + source = &*ips.source; + auto end = source->getBlock()->end(); + if (++ips.source == end) + inliningStack.pop_back(); + } - assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); + // Does the source have regions? If not, use callback to process. + if (source->getNumRegions() == 0) { + assert(builder.saveInsertionPoint().getPoint() == target.getPoint()); // Clone source into insertion point 'target'. - if (failed(process(&source))) + if (failed(process(source))) return failure(); - assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); - } + assert(builder.saveInsertionPoint().getPoint() == target.getPoint()); - // If we've finished inlining this block, pop and continue to next. - if (ips.source == end) { - inliningStack.pop_back(); continue; } - // Otherwise, we have an operation with regions. - auto &sourceWithRegions = *ips.source; - assert(builder.saveInsertionPoint().getPoint() == ips.target.getPoint()); - - // Advance source iterator, we'll handle this operation below. - // If this is the last operation of current level, pop off stack. - if (++ips.source == end) - inliningStack.pop_back(); - // Limited support for region-containing operations. - if (!isa(sourceWithRegions)) - return sourceWithRegions.emitError("unsupported operation '") - << sourceWithRegions.getName() << "' cannot be inlined"; + if (!isa(source)) + return source->emitError("unsupported operation '") + << source->getName() << "' cannot be inlined"; // Note: This does not use cloneAndRename for simplicity, // as there are no annotations, symbols to rename, or names // to prefix. This does mean these operations do not appear // in `il.newOps` for inner-ref renaming walk, FWIW. - auto *newOp = builder.cloneWithoutRegions(sourceWithRegions, mapper); - for (auto [newRegion, oldRegion] : llvm::reverse(llvm::zip_equal( - newOp->getRegions(), sourceWithRegions.getRegions()))) { + auto *newOp = builder.cloneWithoutRegions(*source, mapper); + for (auto [newRegion, oldRegion] : llvm::reverse( + llvm::zip_equal(newOp->getRegions(), source->getRegions()))) { // If region has no blocks, skip. if (oldRegion.empty()) { assert(newRegion.empty()); From b4b4ca4428b8994cea494de31ea75e09b53adf39 Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Thu, 26 Sep 2024 11:12:58 -0500 Subject: [PATCH 3/4] inliner.mlir: add a simpler test, trim and tidy a bit. --- test/Dialect/FIRRTL/inliner.mlir | 62 +++++++++++++++++--------------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/test/Dialect/FIRRTL/inliner.mlir b/test/Dialect/FIRRTL/inliner.mlir index e2aa3796d97d..3a9ef7ff4938 100644 --- a/test/Dialect/FIRRTL/inliner.mlir +++ b/test/Dialect/FIRRTL/inliner.mlir @@ -1270,41 +1270,49 @@ firrtl.module private @test3() { // ----- -// Test inlining into nested layer, and cloning operations with blocks + blockargs (match). +// Directly check simplest example of inlining a module containing a layerblock. -firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} { - firrtl.layer @I inline { - firrtl.layer @J inline { } +firrtl.circuit "InlineLayerBlockSimple" { + firrtl.layer @I inline { } + // CHECK-NOT: @Child + firrtl.module private @Child() attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + firrtl.layerblock @I { + %o = firrtl.wire interesting_name : !firrtl.uint<8> + } } - // CHECK-NOT: @GChild - firrtl.module private @GChild(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { - %0 = firrtl.not %in : (!firrtl.uint<8>) -> !firrtl.uint<8> - firrtl.matchingconnect %out, %0 : !firrtl.uint<8> + // CHECK: @InlineLayerBlockSimple + firrtl.module @InlineLayerBlockSimple() { + // Check inlined structure. + // CHECK-NEXT: layerblock @I + // CHECK-NEXT: firrtl.wire + // CHECK-NEXT: } + firrtl.instance c @Child() } - firrtl.module private @Child(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) { - %gc_in, %gc_out = firrtl.instance gc @GChild(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) - firrtl.matchingconnect %gc_in, %in : !firrtl.uint<8> - firrtl.matchingconnect %out, %gc_out : !firrtl.uint<8> +} + +// ----- + +// Test inlining into nested layer, and cloning operations with blocks + blockargs (match). + +firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} { + firrtl.layer @I inline { + firrtl.layer @J inline { } } // CHECK-NOT: @MatchAgain firrtl.module private @MatchAgain(in %i: !firrtl.enum, None: uint<0>>, out %o: !firrtl.uint<8>) attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { firrtl.match %i : !firrtl.enum, None: uint<0>> { case Some(%arg0) { - %c_in, %c_out = firrtl.instance c @Child(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) - firrtl.matchingconnect %c_in, %arg0 : !firrtl.uint<8> - firrtl.matchingconnect %o, %c_out : !firrtl.uint<8> + %not = firrtl.not %arg0 : (!firrtl.uint<8>) -> !firrtl.uint<8> + firrtl.matchingconnect %o, %not : !firrtl.uint<8> } case None(%arg0) { - %c_in, %c_out = firrtl.instance c @Child(in in: !firrtl.uint<8>, out out: !firrtl.uint<8>) %invalid_ui8 = firrtl.invalidvalue : !firrtl.uint<8> - firrtl.matchingconnect %c_in, %invalid_ui8 : !firrtl.uint<8> - firrtl.matchingconnect %o, %c_out : !firrtl.uint<8> + firrtl.matchingconnect %o, %invalid_ui8 : !firrtl.uint<8> } } } // CHECK: @MatchInline - firrtl.module @MatchInline(in %i: !firrtl.enum, None: uint<0>>, - out %o: !firrtl.probe, @I::@J>) attributes {convention = #firrtl} { + firrtl.module @MatchInline(in %i: !firrtl.enum, None: uint<0>>) { // CHECK-NEXT: layerblock @I firrtl.layerblock @I { // CHECK-NEXT: layerblock @I::@J @@ -1312,15 +1320,10 @@ firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} { // CHECK-NOT: @MatchAgain // CHECK: firrtl.match // CHECK-NEXT: Some(%arg0) - // CHECK-NEXT: %c_c_in, %c_c_out = firrtl.instance c_c @Child - // CHECK-NEXT: firrtl.matchingconnect %c_c_in, %arg0 + // CHECK-NEXT: firrtl.not %arg0 // CHECK: None(%arg0) - // CHECK-NEXT: %c_c_in, %c_c_out = firrtl.instance c_c @Child %c_i, %c_o = firrtl.instance c @MatchAgain(in i: !firrtl.enum, None: uint<0>>, out o: !firrtl.uint<8>) firrtl.matchingconnect %c_i, %i : !firrtl.enum, None: uint<0>> - %0 = firrtl.ref.send %c_o : !firrtl.uint<8> - %1 = firrtl.ref.cast %0 : (!firrtl.probe>) -> !firrtl.probe, @I::@J> - firrtl.ref.define %o, %1 : !firrtl.probe, @I::@J> } } } @@ -1329,10 +1332,13 @@ firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} { // ----- // Test inlining module containing various operations with blocks. +// Include operations before/after regions as well as populating block bodies +// and using results to check inlining actually does work here and the +// management of the insertion points throughout. firrtl.circuit "InlineBlocks" { - firrtl.layer @I inline { - firrtl.layer @J inline { } + firrtl.layer @I inline { + firrtl.layer @J inline { } } firrtl.module private @HasBlocks(in %i: !firrtl.enum, None: uint<0>>, in %cond: !firrtl.uint<1>, From a173744e97a9eb3e8d351e3f4c31c7b558635afb Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Thu, 26 Sep 2024 11:17:31 -0500 Subject: [PATCH 4/4] Add another simpler test. --- test/Dialect/FIRRTL/inliner.mlir | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/Dialect/FIRRTL/inliner.mlir b/test/Dialect/FIRRTL/inliner.mlir index 3a9ef7ff4938..8a1dca859451 100644 --- a/test/Dialect/FIRRTL/inliner.mlir +++ b/test/Dialect/FIRRTL/inliner.mlir @@ -1292,6 +1292,31 @@ firrtl.circuit "InlineLayerBlockSimple" { // ----- +// Check recurse into instances not at top-level. + +firrtl.circuit "WalkIntoInstancesUnderLayerBlock" { + firrtl.layer @I inline { } + // CHECK-NOT: @GChild + firrtl.module private @GChild() attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]} { + %o = firrtl.wire interesting_name : !firrtl.uint<8> + } + // CHECK: @Child + firrtl.module private @Child() { + // CHECK-NEXT: %gc_o = firrtl.wire + firrtl.instance gc @GChild() + } + // CHECK: @WalkIntoInstancesUnderLayerBlock + firrtl.module @WalkIntoInstancesUnderLayerBlock() { + // CHECK-NEXT: layerblock @I + // CHECK-NEXT: firrtl.instance c @Child + firrtl.layerblock @I { + firrtl.instance c @Child() + } + } +} + +// ----- + // Test inlining into nested layer, and cloning operations with blocks + blockargs (match). firrtl.circuit "MatchInline" attributes {enable_layers = [@I]} {