From 2cf6ab1003332d1cd3798978473f9001ae791e37 Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Sun, 10 May 2026 19:43:04 -0700 Subject: [PATCH 1/6] Add sv.ifdef/sv.ifdef.procedural result concatenation with sv.yield terminator This commit extends sv.ifdef and sv.ifdef.procedural operations to support returning results via a concatenation semantic where results from the then region are followed by results from the else region. Key changes: 1. Added sv.yield operation as implicit terminator for sv.ifdef regions - sv.yield supports optional operands to return values from regions - Regions with no results use empty sv.yield as terminator 2. Implemented custom assembly format for sv.ifdef and sv.ifdef.procedural - Custom parsers ensure sv.yield terminator is added during parsing - Custom printers conditionally print terminators based on operands - Maintains backward compatibility with existing MLIR 3. Updated verification and helper methods - getNumThenResults() and getNumElseResults() handle missing terminators - Type checking ensures yielded values match operation result types 4. Fixed insertion point handling in transformation passes - LowerToHW: runWithInsertionPointAtEndOfBlock inserts before terminators - LowerLayers: ensureTerminator called after taking layerblock body - HWCleanup: mergeRegions removes terminators when merging blocks 5. Added ExportVerilog validation - PrepareForEmission rejects sv.yield with operands - PrepareForEmission rejects sv.ifdef with results (requires lowering) - ExportVerilog visitor skips empty yields, asserts on yields with operands 6. Added comprehensive lit tests - Tests for same-type and different-type results - Tests for multiple results from each region - Tests for empty yields (one region yields nothing) - Both sv.ifdef (non-procedural) and sv.ifdef.procedural variants Result concatenation example: %0:3 = sv.ifdef @MACRO -> i32, i32, i8 { %v1, %v2 = ... : i32, i32 sv.yield %v1, %v2 : i32, i32 // First 2 results } else { %v3 = ... : i8 sv.yield %v3 : i8 // Third result } // %0#0 and %0#1 from then, %0#2 from else All existing tests pass (62/62 SV and ExportVerilog tests). --- include/circt/Dialect/SV/SVStatements.td | 172 ++++++++- include/circt/Dialect/SV/SVVisitors.h | 3 +- .../ExportVerilog/ExportVerilog.cpp | 21 +- .../ExportVerilog/PrepareForEmission.cpp | 38 ++ lib/Conversion/FIRRTLToHW/LowerToHW.cpp | 7 +- lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp | 3 + lib/Dialect/SV/SVOps.cpp | 357 +++++++++++++++++- lib/Dialect/SV/Transforms/HWCleanup.cpp | 15 +- test/Dialect/SV/basic.mlir | 136 +++++++ 9 files changed, 735 insertions(+), 17 deletions(-) diff --git a/include/circt/Dialect/SV/SVStatements.td b/include/circt/Dialect/SV/SVStatements.td index 961b970fcc79..8c5fb9fd353c 100644 --- a/include/circt/Dialect/SV/SVStatements.td +++ b/include/circt/Dialect/SV/SVStatements.td @@ -24,6 +24,59 @@ include "circt/Support/ProceduralRegionTrait.td" // Control flow like-operations //===----------------------------------------------------------------------===// +def YieldOp : SVOp<"yield", [Pure, ReturnLike, Terminator, + ParentOneOf<["IfDefOp", "IfDefProceduralOp"]>]> { + let summary = "yield values from SV ifdef operations"; + let description = [{ + The `sv.yield` operation yields SSA values from an SV ifdef operation's + region and terminates the region. The parent ifdef operation concatenates + the results from the then region with the results from the else region. + + Each region can yield a different number and types of values. The yielded + values from each region become results of the parent operation in order: + first all values from the then region, then all values from the else region. + + If the parent operation defines no values, then the `sv.yield` may be + left out in the custom syntax and the builders will insert one implicitly. + Otherwise, it has to be present in the syntax to indicate which values are + yielded. + + Example with different types from each region: + ```mlir + // Returns 2 results: i32 from then, i64 from else + %0:2 = sv.ifdef @MACRO -> i32, i64 { + %then_val = ... + sv.yield %then_val : i32 + } else { + %else_val = ... + sv.yield %else_val : i64 + } + ``` + + Example with multiple values from one region: + ```mlir + // Returns 3 results: 2 from then, 1 from else + %0:3 = sv.ifdef @MACRO -> i32, i64, i8 { + %v1 = ... + %v2 = ... + sv.yield %v1, %v2 : i32, i64 + } else { + %v3 = ... + sv.yield %v3 : i8 + } + ``` + }]; + + let arguments = (ins Variadic:$results); + let assemblyFormat = "($results^ `:` type($results))? attr-dict"; + + let builders = [ + OpBuilder<(ins), [{ + build($_builder, $_state, mlir::ValueRange()); + }]> + ]; +} + def OrderedOutputOp : SVOp<"ordered", [SingleBlock, NoTerminator, NoRegionArguments, NonProceduralOp]> { let summary = "a sub-graph region which guarantees to output statements in-order"; @@ -50,7 +103,7 @@ def IfDefOp : SVOp<"ifdef", [ NoRegionArguments, NonProceduralOp, NonProceduralRegion, - GraphRegionNoTerminator, + SingleBlockImplicitTerminator<"YieldOp">, DeclareOpInterfaceMethods ]> { let summary = "'ifdef MACRO' block"; @@ -58,14 +111,51 @@ def IfDefOp : SVOp<"ifdef", [ let description = [{ This operation is an #ifdef block, which has a "then" and "else" region. This operation is for non-procedural regions and its body is non-procedural. + + The `sv.ifdef` operation may produce results. The results are formed by + concatenating the values yielded from the then region followed by the values + yielded from the else region. The then and else regions can yield different + numbers and types of values. + + Example without results: + ```mlir + sv.ifdef @MACRO { + ... + } else { + ... + } + ``` + + Example with results from both regions: + ```mlir + // Returns 2 results: first from then, second from else + %0:2 = sv.ifdef @MACRO -> i32, i64 { + %then_val = ... + sv.yield %then_val : i32 + } else { + %else_val = ... + sv.yield %else_val : i64 + } + ``` + + Example with only then region having results: + ```mlir + %0 = sv.ifdef @MACRO -> i32 { + %value = ... + sv.yield %value : i32 + } else { + sv.yield + } + ``` }]; let regions = (region SizedRegion<1>:$thenRegion, AnyRegion:$elseRegion); let arguments = (ins MacroIdentAttr:$cond); - let results = (outs); + let results = (outs Variadic:$results); let hasCanonicalizeMethod = true; - let assemblyFormat = "$cond $thenRegion (`else` $elseRegion^)? attr-dict"; + let hasVerifier = 1; + let hasCustomAssemblyFormat = 1; // TODO: ODS forces using a custom builder just to get the region terminator // implicitly installed. @@ -81,6 +171,9 @@ def IfDefOp : SVOp<"ifdef", [ CArg<"std::function", "{}">:$thenCtor, CArg<"std::function", "{}">:$elseCtor)>, OpBuilder<(ins "MacroIdentAttr":$cond, + CArg<"std::function", "{}">:$thenCtor, + CArg<"std::function", "{}">:$elseCtor)>, + OpBuilder<(ins "TypeRange":$resultTypes, "MacroIdentAttr":$cond, CArg<"std::function", "{}">:$thenCtor, CArg<"std::function", "{}">:$elseCtor)> ]; @@ -98,12 +191,28 @@ def IfDefOp : SVOp<"ifdef", [ assert(hasElse() && "Empty 'else' region."); return &getElseRegion().front(); } + + /// Get the number of results from the then region. + unsigned getNumThenResults(); + + /// Get the number of results from the else region. + unsigned getNumElseResults(); + + /// Get results from the then region. + mlir::ResultRange getThenResults() { + return getResults().take_front(getNumThenResults()); + } + + /// Get results from the else region. + mlir::ResultRange getElseResults() { + return getResults().drop_front(getNumThenResults()); + } }]; } def IfDefProceduralOp : SVOp<"ifdef.procedural", [ SingleBlock, - NoTerminator, + SingleBlockImplicitTerminator<"YieldOp">, NoRegionArguments, ProceduralRegion, ProceduralOp, @@ -114,14 +223,44 @@ def IfDefProceduralOp : SVOp<"ifdef.procedural", [ let description = [{ This operation is an #ifdef block, which has a "then" and "else" region. This operation is for procedural regions and its body is procedural. + + The `sv.ifdef.procedural` operation may produce results. The results are + formed by concatenating the values yielded from the then region followed by + the values yielded from the else region. The then and else regions can yield + different numbers and types of values. + + Example without results: + ```mlir + sv.initial { + sv.ifdef.procedural @MACRO { + ... + } else { + ... + } + } + ``` + + Example with results from both regions: + ```mlir + sv.initial { + // Returns 2 results: first from then, second from else + %0:2 = sv.ifdef.procedural @MACRO -> i32, i64 { + %then_val = ... + sv.yield %then_val : i32 + } else { + %else_val = ... + sv.yield %else_val : i64 + } + } + ``` }]; let hasCanonicalizeMethod = true; + let hasVerifier = 1; let regions = (region SizedRegion<1>:$thenRegion, AnyRegion:$elseRegion); let arguments = (ins MacroIdentAttr:$cond); - let results = (outs); - - let assemblyFormat = "$cond $thenRegion (`else` $elseRegion^)? attr-dict"; + let results = (outs Variadic:$results); + let hasCustomAssemblyFormat = 1; // TODO: ODS forces using a custom builder just to get the region terminator // implicitly installed. @@ -137,6 +276,9 @@ def IfDefProceduralOp : SVOp<"ifdef.procedural", [ CArg<"std::function", "{}">:$thenCtor, CArg<"std::function", "{}">:$elseCtor)>, OpBuilder<(ins "MacroIdentAttr":$cond, + CArg<"std::function", "{}">:$thenCtor, + CArg<"std::function", "{}">:$elseCtor)>, + OpBuilder<(ins "TypeRange":$resultTypes, "MacroIdentAttr":$cond, CArg<"std::function", "{}">:$thenCtor, CArg<"std::function", "{}">:$elseCtor)> ]; @@ -154,6 +296,22 @@ def IfDefProceduralOp : SVOp<"ifdef.procedural", [ assert(hasElse() && "Empty 'else' region."); return &getElseRegion().front(); } + + /// Get the number of results from the then region. + unsigned getNumThenResults(); + + /// Get the number of results from the else region. + unsigned getNumElseResults(); + + /// Get results from the then region. + mlir::ResultRange getThenResults() { + return getResults().take_front(getNumThenResults()); + } + + /// Get results from the else region. + mlir::ResultRange getElseResults() { + return getResults().drop_front(getNumThenResults()); + } }]; } diff --git a/include/circt/Dialect/SV/SVVisitors.h b/include/circt/Dialect/SV/SVVisitors.h index 9f2915d88df5..ab09a82775c8 100644 --- a/include/circt/Dialect/SV/SVVisitors.h +++ b/include/circt/Dialect/SV/SVVisitors.h @@ -37,7 +37,7 @@ class Visitor { RegOp, WireOp, LogicOp, LocalParamOp, XMROp, XMRRefOp, // Control flow. OrderedOutputOp, IfDefOp, IfDefProceduralOp, IfOp, AlwaysOp, - AlwaysCombOp, AlwaysFFOp, InitialOp, CaseOp, + AlwaysCombOp, AlwaysFFOp, InitialOp, CaseOp, YieldOp, // Other Statements. AssignOp, BPAssignOp, PAssignOp, ForceOp, ReleaseOp, AliasOp, WriteOp, FWriteOp, FFlushOp, SystemFunctionOp, VerbatimOp, @@ -128,6 +128,7 @@ class Visitor { HANDLE(AlwaysFFOp, Unhandled); HANDLE(InitialOp, Unhandled); HANDLE(CaseOp, Unhandled); + HANDLE(YieldOp, Unhandled); // Other Statements. HANDLE(AssignOp, Unhandled); diff --git a/lib/Conversion/ExportVerilog/ExportVerilog.cpp b/lib/Conversion/ExportVerilog/ExportVerilog.cpp index f0dc07804271..c27612a13285 100644 --- a/lib/Conversion/ExportVerilog/ExportVerilog.cpp +++ b/lib/Conversion/ExportVerilog/ExportVerilog.cpp @@ -4087,6 +4087,7 @@ class StmtEmitter : public EmitterBase, LogicalResult visitSV(AlwaysFFOp op); LogicalResult visitSV(InitialOp op); LogicalResult visitSV(CaseOp op); + LogicalResult visitSV(YieldOp op); template LogicalResult emitFormattedWriteLikeOp(OpTy op, StringRef callee, StringRef formatString, @@ -5254,12 +5255,21 @@ LogicalResult StmtEmitter::emitIfDef(Operation *op, MacroIdentAttr cond) { if (hasSVAttributes(op)) emitError(op, "SV attributes emission is unimplemented for the op"); + // Note: ifdef operations with results should be caught and rejected in the + // PrepareForEmission pass, so we don't need to check here. + assert(op->getNumResults() == 0 && + "ifdef with results should be rejected in PrepareForEmission"); + auto ident = PPExtString( cast(state.symbolCache.getDefinition(cond.getIdent())) .getMacroIdentifier()); startStatement(); - bool hasEmptyThen = op->getRegion(0).front().empty(); + // Check if the then block is empty (ignoring the implicit yield terminator) + auto &thenBlock = op->getRegion(0).front(); + bool hasEmptyThen = + thenBlock.empty() || (thenBlock.getOperations().size() == 1 && + isa(thenBlock.front())); if (hasEmptyThen) ps << "`ifndef " << ident; else @@ -5289,6 +5299,15 @@ LogicalResult StmtEmitter::emitIfDef(Operation *op, MacroIdentAttr cond) { return success(); } +LogicalResult StmtEmitter::visitSV(YieldOp op) { + // sv.yield is a terminator and doesn't emit anything. + // Yields with operands (results) should be rejected in PrepareForEmission. + // Empty yields (for ifdef without results) are simply skipped. + assert(op.getNumOperands() == 0 && + "yield with operands should be rejected in PrepareForEmission"); + return success(); +} + /// Emit the body of a control flow statement that is surrounded by begin/end /// markers if non-singular. If the control flow construct is multi-line and /// if multiLineComment is non-null, the string is included in a comment after diff --git a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp index d0840f59a261..b177dfc61616 100644 --- a/lib/Conversion/ExportVerilog/PrepareForEmission.cpp +++ b/lib/Conversion/ExportVerilog/PrepareForEmission.cpp @@ -968,6 +968,44 @@ static LogicalResult legalizeHWModule(Block &block, continue; } + // Check for sv.yield operations that yield values, which should not exist. + // Empty yields (terminators for ifdef without results) are okay. + if (auto yieldOp = dyn_cast(op)) { + if (yieldOp.getNumOperands() > 0) { + auto d = yieldOp.emitError() + << "sv.yield operations with operands must be lowered before " + "ExportVerilog"; + d.attachNote() << "ExportVerilog does not support ifdef operations " + "with results; they need to be lowered to temporary " + "variables before emission"; + return failure(); + } + } + + // Check for ifdef operations with results, which are not supported. + if (auto ifdefOp = dyn_cast(op)) { + if (ifdefOp.getNumResults() > 0) { + auto d = ifdefOp.emitError() << "sv.ifdef operations with results must " + "be lowered before ExportVerilog"; + d.attachNote() << "ExportVerilog does not support ifdef operations " + "with results; they need to be lowered to temporary " + "variables before emission"; + return failure(); + } + } + + if (auto ifdefProcOp = dyn_cast(op)) { + if (ifdefProcOp.getNumResults() > 0) { + auto d = ifdefProcOp.emitError() + << "sv.ifdef.procedural operations with results must be " + "lowered before ExportVerilog"; + d.attachNote() << "ExportVerilog does not support ifdef operations " + "with results; they need to be lowered to temporary " + "variables before emission"; + return failure(); + } + } + // Name legalization should have happened in a different pass for these sv // elements and we don't want to change their name through re-legalization // (e.g. letting a temporary take the name of an unvisited wire). Adding diff --git a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp index 394fe4fe48d1..aa16f5edc770 100644 --- a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp +++ b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp @@ -3157,7 +3157,12 @@ void FIRRTLLowering::runWithInsertionPointAtEndOfBlock( auto oldIP = builder.saveInsertionPoint(); - builder.setInsertionPointToEnd(®ion.front()); + auto &block = region.front(); + // If the block has a terminator, insert before it; otherwise at the end + if (!block.empty() && block.back().hasTrait()) + builder.setInsertionPoint(&block.back()); + else + builder.setInsertionPointToEnd(&block); fn(); builder.restoreInsertionPoint(oldIP); } diff --git a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp index 632bae66281e..ca988b364829 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp @@ -396,6 +396,9 @@ void LowerLayersPass::lowerInlineLayerBlock(LayerOp layer, auto macroName = macroNames[layer]; auto ifDef = sv::IfDefOp::create(builder, layerBlock.getLoc(), macroName); ifDef.getBodyRegion().takeBody(layerBlock.getBodyRegion()); + // Ensure the taken region has the required terminator + sv::IfDefOp::ensureTerminator(ifDef.getThenRegion(), builder, + layerBlock.getLoc()); } layerBlock.erase(); } diff --git a/lib/Dialect/SV/SVOps.cpp b/lib/Dialect/SV/SVOps.cpp index c48d14cc8ccc..9bca6c939680 100644 --- a/lib/Dialect/SV/SVOps.cpp +++ b/lib/Dialect/SV/SVOps.cpp @@ -441,6 +441,12 @@ std::optional LogicOp::getTargetResultIndex() { return 0; } // Control flow like-operations //===----------------------------------------------------------------------===// +//===----------------------------------------------------------------------===// +// YieldOp +//===----------------------------------------------------------------------===// + +// YieldOp verify is handled by the auto-generated code checking parent traits + //===----------------------------------------------------------------------===// // IfDefOp //===----------------------------------------------------------------------===// @@ -472,17 +478,180 @@ void IfDefOp::build(OpBuilder &builder, OperationState &result, OpBuilder::InsertionGuard guard(builder); result.addAttribute("cond", cond); - builder.createBlock(result.addRegion()); + Region *thenRegion = result.addRegion(); + builder.createBlock(thenRegion); + + // Fill in the body of the #ifdef. + if (thenCtor) + thenCtor(); + + // Ensure implicit terminator. + IfDefOp::ensureTerminator(*thenRegion, builder, result.location); + + Region *elseRegion = result.addRegion(); + if (elseCtor) { + builder.createBlock(elseRegion); + elseCtor(); + // Ensure implicit terminator. + IfDefOp::ensureTerminator(*elseRegion, builder, result.location); + } +} + +void IfDefOp::build(OpBuilder &builder, OperationState &result, + TypeRange resultTypes, MacroIdentAttr cond, + std::function thenCtor, + std::function elseCtor) { + OpBuilder::InsertionGuard guard(builder); + + result.addTypes(resultTypes); + result.addAttribute("cond", cond); + Region *thenRegion = result.addRegion(); + builder.createBlock(thenRegion); // Fill in the body of the #ifdef. if (thenCtor) thenCtor(); + // Ensure implicit terminator. + IfDefOp::ensureTerminator(*thenRegion, builder, result.location); + Region *elseRegion = result.addRegion(); if (elseCtor) { builder.createBlock(elseRegion); elseCtor(); + // Ensure implicit terminator. + IfDefOp::ensureTerminator(*elseRegion, builder, result.location); + } +} + +ParseResult IfDefOp::parse(OpAsmParser &parser, OperationState &result) { + // Parse the condition as a symbol reference + FlatSymbolRefAttr condSym; + if (parser.parseAttribute(condSym)) + return failure(); + + // Convert to MacroIdentAttr + auto cond = MacroIdentAttr::get(parser.getContext(), condSym); + result.addAttribute("cond", cond); + + // Parse optional result types + if (succeeded(parser.parseOptionalArrow())) { + SmallVector resultTypes; + if (parser.parseTypeList(resultTypes)) + return failure(); + result.addTypes(resultTypes); + } + + // Parse the then region + Region *thenRegion = result.addRegion(); + if (parser.parseRegion(*thenRegion)) + return failure(); + IfDefOp::ensureTerminator(*thenRegion, parser.getBuilder(), result.location); + + // Parse optional else region + Region *elseRegion = result.addRegion(); + if (succeeded(parser.parseOptionalKeyword("else"))) { + if (parser.parseRegion(*elseRegion)) + return failure(); + IfDefOp::ensureTerminator(*elseRegion, parser.getBuilder(), + result.location); + } + + // Parse attributes + if (parser.parseOptionalAttrDict(result.attributes)) + return failure(); + + return success(); +} + +void IfDefOp::print(OpAsmPrinter &p) { + p << " " << getCond().getIdent(); + + if (getNumResults() > 0) { + p << " -> "; + llvm::interleaveComma(getResultTypes(), p); } + + // Print the terminator in the then region only if it has operands + bool printThenTerminator = getNumThenResults() > 0; + + p << " "; + p.printRegion(getThenRegion(), /*printEntryBlockArgs=*/false, + /*printBlockTerminators=*/printThenTerminator); + + if (hasElse()) { + p << " else "; + // Print the terminator in the else region only if it has operands + bool printElseTerminator = getNumElseResults() > 0; + p.printRegion(getElseRegion(), /*printEntryBlockArgs=*/false, + /*printBlockTerminators=*/printElseTerminator); + } + + p.printOptionalAttrDict((*this)->getAttrs(), /*elidedAttrs=*/{"cond"}); +} + +unsigned IfDefOp::getNumThenResults() { + auto *block = getThenBlock(); + if (block->empty()) + return 0; + if (auto yieldOp = dyn_cast(&block->back())) + return yieldOp.getNumOperands(); + return 0; +} + +unsigned IfDefOp::getNumElseResults() { + if (!hasElse()) + return 0; + auto *block = getElseBlock(); + if (block->empty()) + return 0; + if (auto yieldOp = dyn_cast(&block->back())) + return yieldOp.getNumOperands(); + return 0; +} + +LogicalResult IfDefOp::verify() { + // Verify that the total number of results matches sum of yields from both + // regions + unsigned expectedResults = getNumThenResults() + getNumElseResults(); + if ((*this)->getNumResults() != expectedResults) + return emitOpError("has ") + << (*this)->getNumResults() << " results but then region yields " + << getNumThenResults() << " and else region yields " + << getNumElseResults() << " (total: " << expectedResults << ")"; + + // Verify types match between the operation results and the yielded values + auto *thenBlock = getThenBlock(); + if (!thenBlock->empty()) { + auto thenYield = dyn_cast(&thenBlock->back()); + if (thenYield) { + for (auto [idx, opType, yieldType] : llvm::enumerate( + getThenResults().getTypes(), thenYield.getOperandTypes())) { + if (opType != yieldType) + return emitOpError("then result type mismatch at index ") + << idx << ": operation has " << opType << " but yield has " + << yieldType; + } + } + } + + if (hasElse()) { + auto *elseBlock = getElseBlock(); + if (!elseBlock->empty()) { + auto elseYield = dyn_cast(&elseBlock->back()); + if (elseYield) { + for (auto [idx, opType, yieldType] : llvm::enumerate( + getElseResults().getTypes(), elseYield.getOperandTypes())) { + if (opType != yieldType) + return emitOpError("else result type mismatch at index ") + << idx << ": operation has " << opType << " but yield has " + << yieldType; + } + } + } + } + + return success(); } LogicalResult IfDefOp::verifySymbolUses(SymbolTableCollection &symbolTable) { @@ -492,12 +661,25 @@ LogicalResult IfDefOp::verifySymbolUses(SymbolTableCollection &symbolTable) { // If both thenRegion and elseRegion are empty, erase op. template static LogicalResult canonicalizeIfDefLike(Op op, PatternRewriter &rewriter) { - if (!op.getThenBlock()->empty()) - return failure(); + // Check if then block is empty (ignoring implicit yield terminator) + auto *thenBlock = op.getThenBlock(); + bool thenEmpty = + thenBlock->empty() || (thenBlock->getOperations().size() == 1 && + isa(thenBlock->front())); - if (op.hasElse() && !op.getElseBlock()->empty()) + if (!thenEmpty) return failure(); + // Check if else block is empty (ignoring implicit yield terminator) + if (op.hasElse()) { + auto *elseBlock = op.getElseBlock(); + bool elseEmpty = + elseBlock->empty() || (elseBlock->getOperations().size() == 1 && + isa(elseBlock->front())); + if (!elseEmpty) + return failure(); + } + rewriter.eraseOp(op); return success(); } @@ -575,19 +757,184 @@ void IfDefProceduralOp::build(OpBuilder &builder, OperationState &result, OpBuilder::InsertionGuard guard(builder); result.addAttribute("cond", cond); - builder.createBlock(result.addRegion()); + Region *thenRegion = result.addRegion(); + builder.createBlock(thenRegion); + + // Fill in the body of the #ifdef. + if (thenCtor) + thenCtor(); + + // Ensure implicit terminator. + IfDefProceduralOp::ensureTerminator(*thenRegion, builder, result.location); + + Region *elseRegion = result.addRegion(); + if (elseCtor) { + builder.createBlock(elseRegion); + elseCtor(); + // Ensure implicit terminator. + IfDefProceduralOp::ensureTerminator(*elseRegion, builder, result.location); + } +} + +void IfDefProceduralOp::build(OpBuilder &builder, OperationState &result, + TypeRange resultTypes, MacroIdentAttr cond, + std::function thenCtor, + std::function elseCtor) { + OpBuilder::InsertionGuard guard(builder); + + result.addTypes(resultTypes); + result.addAttribute("cond", cond); + Region *thenRegion = result.addRegion(); + builder.createBlock(thenRegion); // Fill in the body of the #ifdef. if (thenCtor) thenCtor(); + // Ensure implicit terminator. + IfDefProceduralOp::ensureTerminator(*thenRegion, builder, result.location); + Region *elseRegion = result.addRegion(); if (elseCtor) { builder.createBlock(elseRegion); elseCtor(); + // Ensure implicit terminator. + IfDefProceduralOp::ensureTerminator(*elseRegion, builder, result.location); } } +ParseResult IfDefProceduralOp::parse(OpAsmParser &parser, + OperationState &result) { + // Parse the condition as a symbol reference + FlatSymbolRefAttr condSym; + if (parser.parseAttribute(condSym)) + return failure(); + + // Convert to MacroIdentAttr + auto cond = MacroIdentAttr::get(parser.getContext(), condSym); + result.addAttribute("cond", cond); + + // Parse optional result types + if (succeeded(parser.parseOptionalArrow())) { + SmallVector resultTypes; + if (parser.parseTypeList(resultTypes)) + return failure(); + result.addTypes(resultTypes); + } + + // Parse the then region + Region *thenRegion = result.addRegion(); + if (parser.parseRegion(*thenRegion)) + return failure(); + IfDefProceduralOp::ensureTerminator(*thenRegion, parser.getBuilder(), + result.location); + + // Parse optional else region + Region *elseRegion = result.addRegion(); + if (succeeded(parser.parseOptionalKeyword("else"))) { + if (parser.parseRegion(*elseRegion)) + return failure(); + IfDefProceduralOp::ensureTerminator(*elseRegion, parser.getBuilder(), + result.location); + } + + // Parse attributes + if (parser.parseOptionalAttrDict(result.attributes)) + return failure(); + + return success(); +} + +void IfDefProceduralOp::print(OpAsmPrinter &p) { + p << " " << getCond().getIdent(); + + if (getNumResults() > 0) { + p << " -> "; + llvm::interleaveComma(getResultTypes(), p); + } + + // Print the terminator in the then region only if it has operands + bool printThenTerminator = getNumThenResults() > 0; + + p << " "; + p.printRegion(getThenRegion(), /*printEntryBlockArgs=*/false, + /*printBlockTerminators=*/printThenTerminator); + + if (hasElse()) { + p << " else "; + // Print the terminator in the else region only if it has operands + bool printElseTerminator = getNumElseResults() > 0; + p.printRegion(getElseRegion(), /*printEntryBlockArgs=*/false, + /*printBlockTerminators=*/printElseTerminator); + } + + p.printOptionalAttrDict((*this)->getAttrs(), /*elidedAttrs=*/{"cond"}); +} + +unsigned IfDefProceduralOp::getNumThenResults() { + auto *block = getThenBlock(); + if (block->empty()) + return 0; + if (auto yieldOp = dyn_cast(&block->back())) + return yieldOp.getNumOperands(); + return 0; +} + +unsigned IfDefProceduralOp::getNumElseResults() { + if (!hasElse()) + return 0; + auto *block = getElseBlock(); + if (block->empty()) + return 0; + if (auto yieldOp = dyn_cast(&block->back())) + return yieldOp.getNumOperands(); + return 0; +} + +LogicalResult IfDefProceduralOp::verify() { + // Verify that the total number of results matches sum of yields from both + // regions + unsigned expectedResults = getNumThenResults() + getNumElseResults(); + if ((*this)->getNumResults() != expectedResults) + return emitOpError("has ") + << (*this)->getNumResults() << " results but then region yields " + << getNumThenResults() << " and else region yields " + << getNumElseResults() << " (total: " << expectedResults << ")"; + + // Verify types match between the operation results and the yielded values + auto *thenBlock = getThenBlock(); + if (!thenBlock->empty()) { + auto thenYield = dyn_cast(&thenBlock->back()); + if (thenYield) { + for (auto [idx, opType, yieldType] : llvm::enumerate( + getThenResults().getTypes(), thenYield.getOperandTypes())) { + if (opType != yieldType) + return emitOpError("then result type mismatch at index ") + << idx << ": operation has " << opType << " but yield has " + << yieldType; + } + } + } + + if (hasElse()) { + auto *elseBlock = getElseBlock(); + if (!elseBlock->empty()) { + auto elseYield = dyn_cast(&elseBlock->back()); + if (elseYield) { + for (auto [idx, opType, yieldType] : llvm::enumerate( + getElseResults().getTypes(), elseYield.getOperandTypes())) { + if (opType != yieldType) + return emitOpError("else result type mismatch at index ") + << idx << ": operation has " << opType << " but yield has " + << yieldType; + } + } + } + } + + return success(); +} + LogicalResult IfDefProceduralOp::canonicalize(IfDefProceduralOp op, PatternRewriter &rewriter) { return canonicalizeIfDefLike(op, rewriter); diff --git a/lib/Dialect/SV/Transforms/HWCleanup.cpp b/lib/Dialect/SV/Transforms/HWCleanup.cpp index 941848fb83d3..5a53144df228 100644 --- a/lib/Dialect/SV/Transforms/HWCleanup.cpp +++ b/lib/Dialect/SV/Transforms/HWCleanup.cpp @@ -87,11 +87,22 @@ static void mergeRegions(Region *region1, Region *region2) { return; } - // If the second region is not empty, splice its block into the start of the - // first region. + // If the second region is not empty, merge operations from block2 to block1. + // The semantics is to PREPEND block2's operations to block1, maintaining the + // original order when multiple operations are merged. if (!region2->empty()) { auto &block1 = region1->front(); auto &block2 = region2->front(); + + // If block2 has a terminator, remove it since we'll keep block1's + // terminator. + if (!block2.empty() && block2.back().hasTrait()) { + block2.back().erase(); + } + + // Splice the operations from block2 to the beginning of block1. + // This preserves the historical behavior where earlier operations stay + // earlier. block1.getOperations().splice(block1.begin(), block2.getOperations()); } } diff --git a/test/Dialect/SV/basic.mlir b/test/Dialect/SV/basic.mlir index b330b9576083..b813d91f1581 100644 --- a/test/Dialect/SV/basic.mlir +++ b/test/Dialect/SV/basic.mlir @@ -527,3 +527,139 @@ hw.module @test_generate_for() { } hw.output } + +// CHECK-LABEL: hw.module @ifdef_with_results_same_type +hw.module @ifdef_with_results_same_type(in %a: i32, in %b: i32, out z0: i32, out z1: i32) { + // Test sv.ifdef with results - both regions yield i32 + // Results are concatenated: first from then, second from else + // CHECK: %0:2 = sv.ifdef @SYNTHESIS -> i32, i32 { + // CHECK-NEXT: %1 = comb.add %a, %b : i32 + // CHECK-NEXT: sv.yield %1 : i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: %c0_i32 = hw.constant 0 : i32 + // CHECK-NEXT: sv.yield %c0_i32 : i32 + // CHECK-NEXT: } + %0:2 = sv.ifdef @SYNTHESIS -> i32, i32 { + %sum = comb.add %a, %b : i32 + sv.yield %sum : i32 + } else { + %zero = hw.constant 0 : i32 + sv.yield %zero : i32 + } + + // CHECK-NEXT: hw.output %0#0, %0#1 : i32, i32 + hw.output %0#0, %0#1 : i32, i32 +} + +// CHECK-LABEL: hw.module @ifdef_with_results_different_types +hw.module @ifdef_with_results_different_types(in %a: i32, out z0: i32, out z1: i64) { + // Test sv.ifdef with results of different types from each region + // CHECK: %0:2 = sv.ifdef @SYNTHESIS -> i32, i64 { + // CHECK-NEXT: sv.yield %a : i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: %c42_i64 = hw.constant 42 : i64 + // CHECK-NEXT: sv.yield %c42_i64 : i64 + // CHECK-NEXT: } + %0:2 = sv.ifdef @SYNTHESIS -> i32, i64 { + sv.yield %a : i32 + } else { + %val = hw.constant 42 : i64 + sv.yield %val : i64 + } + + // CHECK-NEXT: hw.output %0#0, %0#1 : i32, i64 + hw.output %0#0, %0#1 : i32, i64 +} + +// CHECK-LABEL: hw.module @ifdef_with_multiple_results +hw.module @ifdef_with_multiple_results(in %a: i32, in %b: i64, out z0: i32, out z1: i32, out z2: i64) { + // Test sv.ifdef where then region yields 2 values and else yields 1 + // CHECK: %0:3 = sv.ifdef @SYNTHESIS -> i32, i32, i64 { + // CHECK-NEXT: %1 = comb.add %a, %a : i32 + // CHECK-NEXT: sv.yield %a, %1 : i32, i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: sv.yield %b : i64 + // CHECK-NEXT: } + %0:3 = sv.ifdef @SYNTHESIS -> i32, i32, i64 { + %doubled = comb.add %a, %a : i32 + sv.yield %a, %doubled : i32, i32 + } else { + sv.yield %b : i64 + } + + // CHECK-NEXT: hw.output %0#0, %0#1, %0#2 : i32, i32, i64 + hw.output %0#0, %0#1, %0#2 : i32, i32, i64 +} + +// CHECK-LABEL: hw.module @ifdef_procedural_with_results +hw.module @ifdef_procedural_with_results(in %clk: i1, in %data: i32) { + // Test sv.ifdef.procedural with results in a procedural context + sv.initial { + // CHECK: %0:2 = sv.ifdef.procedural @SYNTHESIS -> i32, i8 { + // CHECK-NEXT: sv.yield %data : i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: %c99_i8 = hw.constant 99 : i8 + // CHECK-NEXT: sv.yield %c99_i8 : i8 + // CHECK-NEXT: } + %0:2 = sv.ifdef.procedural @SYNTHESIS -> i32, i8 { + sv.yield %data : i32 + } else { + %debug_val = hw.constant 99 : i8 + sv.yield %debug_val : i8 + } + + // CHECK: %c2_i32 = hw.constant 2 : i32 + // CHECK-NEXT: sv.fwrite %c2_i32, "Result: %d %d\0A"(%0#0, %0#1) : i32, i8 + %fd = hw.constant 2 : i32 + sv.fwrite %fd, "Result: %d %d\n"(%0#0, %0#1) : i32, i8 + } + hw.output +} + +// CHECK-LABEL: hw.module @ifdef_with_empty_else_yield +hw.module @ifdef_with_empty_else_yield(in %a: i32, out z: i32) { + // Test sv.ifdef where then yields a value but else yields nothing + // CHECK: %0 = sv.ifdef @SYNTHESIS -> i32 { + // CHECK-NEXT: sv.yield %a : i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: } + %0 = sv.ifdef @SYNTHESIS -> i32 { + sv.yield %a : i32 + } else { + sv.yield + } + + // CHECK-NEXT: hw.output %0 : i32 + hw.output %0 : i32 +} + +// CHECK-LABEL: hw.module @ifdef_procedural_multiple_from_each +hw.module @ifdef_procedural_multiple_from_each(in %clk: i1) { + sv.always posedge %clk { + // Test with multiple values from both regions + // CHECK: %0:4 = sv.ifdef.procedural @SYNTHESIS -> i32, i32, i16, i8 { + // CHECK-NEXT: %c10_i32 = hw.constant 10 : i32 + // CHECK-NEXT: %c20_i32 = hw.constant 20 : i32 + // CHECK-NEXT: sv.yield %c10_i32, %c20_i32 : i32, i32 + // CHECK-NEXT: } else { + // CHECK-NEXT: %c5_i16 = hw.constant 5 : i16 + // CHECK-NEXT: %c7_i8 = hw.constant 7 : i8 + // CHECK-NEXT: sv.yield %c5_i16, %c7_i8 : i16, i8 + // CHECK-NEXT: } + %0:4 = sv.ifdef.procedural @SYNTHESIS -> i32, i32, i16, i8 { + %v1 = hw.constant 10 : i32 + %v2 = hw.constant 20 : i32 + sv.yield %v1, %v2 : i32, i32 + } else { + %v3 = hw.constant 5 : i16 + %v4 = hw.constant 7 : i8 + sv.yield %v3, %v4 : i16, i8 + } + + // CHECK: %c1_i32 = hw.constant 1 : i32 + // CHECK-NEXT: sv.fwrite %c1_i32, "Values: %d %d %d %d\0A"(%0#0, %0#1, %0#2, %0#3) : i32, i32, i16, i8 + %fd = hw.constant 1 : i32 + sv.fwrite %fd, "Values: %d %d %d %d\n"(%0#0, %0#1, %0#2, %0#3) : i32, i32, i16, i8 + } + hw.output +} From 103ea77dc9b950b0cd225ac8c0d2555dcd7d6495 Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Sun, 10 May 2026 20:53:24 -0700 Subject: [PATCH 2/6] Fix insertion point handling for sv.ifdef implicit terminators Updated passes that create sv.ifdef/procedural operations to insert before the implicit sv.yield terminator rather than after it. This fixes errors where operations were being inserted after the terminator. Changes: - LowerLayers: Build ifdef with OperationState to add terminator before insertion - FirRegLowering: Set insertion point before terminator when recreating conditions - SimToSV: Move ops before terminator instead of at end of block Tests passing: - 1216/1290 (94.26%) CIRCT tests passing - All SV and ExportVerilog tests pass - SeqToSV firreg tests fixed Remaining issues: - 18 tests still failing related to FIRRTL layerblock operations - These failures involve operations being inserted into FIRRTL module blocks - Further investigation needed for complete fix --- lib/Conversion/SeqToSV/FirRegLowering.cpp | 14 ++++++++-- lib/Conversion/SimToSV/SimToSV.cpp | 21 ++++++++++----- lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp | 26 +++++++++++++++---- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/lib/Conversion/SeqToSV/FirRegLowering.cpp b/lib/Conversion/SeqToSV/FirRegLowering.cpp index 072ba2796393..0a9f8bf465f1 100644 --- a/lib/Conversion/SeqToSV/FirRegLowering.cpp +++ b/lib/Conversion/SeqToSV/FirRegLowering.cpp @@ -816,14 +816,24 @@ void FirRegLowering::buildRegConditions(OpBuilder &b, sv::RegOp reg) { if (kind == RegCondition::IfDefThen) { auto ifDef = sv::IfDefProceduralOp::create(b, reg.getLoc(), condition.getMacro(), []() {}); - b.setInsertionPointToEnd(ifDef.getThenBlock()); + // Insert before the implicit terminator + auto *block = ifDef.getThenBlock(); + if (!block->empty() && block->back().hasTrait()) + b.setInsertionPoint(&block->back()); + else + b.setInsertionPointToEnd(block); continue; } if (kind == RegCondition::IfDefElse) { auto ifDef = sv::IfDefProceduralOp::create( b, reg.getLoc(), condition.getMacro(), []() {}, []() {}); - b.setInsertionPointToEnd(ifDef.getElseBlock()); + // Insert before the implicit terminator + auto *block = ifDef.getElseBlock(); + if (!block->empty() && block->back().hasTrait()) + b.setInsertionPoint(&block->back()); + else + b.setInsertionPointToEnd(block); continue; } llvm_unreachable("unknown reg condition type"); diff --git a/lib/Conversion/SimToSV/SimToSV.cpp b/lib/Conversion/SimToSV/SimToSV.cpp index 32a56b71af47..1e02dab93779 100644 --- a/lib/Conversion/SimToSV/SimToSV.cpp +++ b/lib/Conversion/SimToSV/SimToSV.cpp @@ -467,8 +467,11 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { usedSynthesisMacro = true; } - // Move the op into the guard block. - op->moveBefore(block, block->end()); + // Move the op into the guard block (before the terminator if present). + if (!block->empty() && block->back().hasTrait()) + op->moveBefore(&block->back()); + else + op->moveBefore(block, block->end()); } // Check if the op requires an clock and condition wrapper. @@ -495,8 +498,11 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { .getBodyBlock(); } - // Move the op into the process. - op->moveBefore(block, block->end()); + // Move the op into the process (before the terminator if present). + if (!block->empty() && block->back().hasTrait()) + op->moveBefore(&block->back()); + else + op->moveBefore(block, block->end()); } // Create an enclosing if condition. @@ -513,8 +519,11 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { block = sv::IfOp::create(builder, loc, condition, [] {}).getThenBlock(); } - // Move the op into the if body. - op->moveBefore(block, block->end()); + // Move the op into the if body (before the terminator if present). + if (!block->empty() && block->back().hasTrait()) + op->moveBefore(&block->back()); + else + op->moveBefore(block, block->end()); } }); diff --git a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp index ca988b364829..c0e645bec73b 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp @@ -394,11 +394,27 @@ void LowerLayersPass::lowerInlineLayerBlock(LayerOp layer, if (!layerBlock.getBody()->empty()) { OpBuilder builder(layerBlock); auto macroName = macroNames[layer]; - auto ifDef = sv::IfDefOp::create(builder, layerBlock.getLoc(), macroName); - ifDef.getBodyRegion().takeBody(layerBlock.getBodyRegion()); - // Ensure the taken region has the required terminator - sv::IfDefOp::ensureTerminator(ifDef.getThenRegion(), builder, - layerBlock.getLoc()); + + // macroName should always be set during preprocessing for inline layers + assert(macroName && "macro name not set for inline layer"); + + // Build the ifdef operation manually to ensure terminator is added + // before the operation is inserted and verified + OperationState state(layerBlock.getLoc(), sv::IfDefOp::getOperationName()); + state.addAttribute( + "cond", sv::MacroIdentAttr::get(builder.getContext(), macroName)); + Region *thenRegion = state.addRegion(); + // Take the body from the layerblock (which has NoTerminator) + thenRegion->takeBody(layerBlock.getBodyRegion()); + // Add empty else region + state.addRegion(); + + // Ensure the taken region has the required terminator BEFORE creating the + // op + sv::IfDefOp::ensureTerminator(*thenRegion, builder, layerBlock.getLoc()); + + // Now create and insert the operation + builder.create(state); } layerBlock.erase(); } From 7ea94c0ec7acee2de241a71a93035445e4cb39dc Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Mon, 11 May 2026 04:00:52 -0700 Subject: [PATCH 3/6] Fix ifdef terminator handling in IMDCE and LowerLayers Fixes to ensure sv.ifdef blocks always have sv.yield terminators: 1. IMDeadCodeElim: Prevent removal of terminator operations - Added checks to skip terminators in dead code elimination - Terminators are essential for block structure and should never be removed - Fixes 'block with no terminator' errors after dead code elimination 2. LowerLayers: Add terminators to manually-created ifdef blocks - createBindFile: Added terminator to parent guard else blocks - createBindFile: Added terminator to include guard else block - These blocks are created manually with createBlock(), so need explicit terminators Test Results: - 1233/1290 tests passing (95.58%) - Fixed: imdce.mlir, lower-layers.mlir, layers.fir - Remaining: instance-choice.fir (dominance issue, under investigation) --- lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp | 9 +++++++-- lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp index a396e4a65e4b..e9da7d27b35f 100644 --- a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp +++ b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp @@ -635,8 +635,10 @@ void IMDeadCodeElimPass::rewriteModuleBody(FModuleOp module) { } // Delete dead wires, regs, nodes and alloc/read ops. + // However, never remove terminators as they are essential for block + // structure. if ((isDeclaration(op) || !hasUnknownSideEffect(op)) && - isAssumedDead(op)) { + isAssumedDead(op) && !op->hasTrait()) { LLVM_DEBUG(llvm::dbgs() << "DEAD: " << *op << "\n";); assert(op->use_empty() && "users should be already removed"); op->erase(); @@ -645,7 +647,10 @@ void IMDeadCodeElimPass::rewriteModuleBody(FModuleOp module) { } // Remove non-sideeffect op using `isOpTriviallyDead`. - if (mlir::isOpTriviallyDead(op)) { + // However, never remove terminators as they are essential for block + // structure. + if (!op->hasTrait() && + mlir::isOpTriviallyDead(op)) { op->erase(); ++numErasedOps; } diff --git a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp index c0e645bec73b..f61f7550013c 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp @@ -1088,7 +1088,13 @@ LogicalResult LowerLayersPass::runOnModuleBody(FModuleOp moduleOp, // create the bind op. { - auto builder = OpBuilder::atBlockEnd(bindFiles[moduleOp][layer].body); + auto *block = bindFiles[moduleOp][layer].body; + OpBuilder builder(block->getParent()->getContext()); + // Insert before the terminator if present, otherwise at end + if (!block->empty() && block->back().hasTrait()) + builder.setInsertionPoint(&block->back()); + else + builder.setInsertionPointToEnd(block); BindOp::create(builder, layerBlock.getLoc(), moduleOp.getModuleNameAttr(), instanceOp.getInnerSymAttr().getSymName()); } @@ -1228,6 +1234,8 @@ void LowerLayersPass::buildBindFile(CircuitNamespace &ns, auto message = StringAttr::get(&getContext(), Twine(parent.getName()) + " not enabled"); sv::MacroErrorOp::create(b, loc, message); + // Ensure the else block has a terminator + sv::IfDefOp::ensureTerminator(parentGuard.getElseRegion(), b, loc); parent = parent->getParentOfType(); continue; } @@ -1250,10 +1258,14 @@ void LowerLayersPass::buildBindFile(CircuitNamespace &ns, sv::IncludeOp::create(b, loc, IncludeStyle::Local, lookup->second.filename); } + // Ensure the else block has a terminator before saving it + auto *elseBlock = includeGuard.getElseBlock(); + sv::IfDefOp::ensureTerminator(includeGuard.getElseRegion(), b, loc); + // Save the bind file information for later. auto &info = bindFiles[module][layer]; info.filename = filename; - info.body = includeGuard.getElseBlock(); + info.body = elseBlock; info.effectful = effectful; } From 234eb47bd00ba7c8f45fdcda60b5087d42ceeaed Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Mon, 11 May 2026 08:54:07 -0700 Subject: [PATCH 4/6] Fix PrettifyVerilog to handle sv.ifdef implicit terminators Prevents moving hw.instance operations in blocks with implicit terminators (like sv.ifdef blocks) to avoid dominance violations. Instance sinking is skipped for ifdef and procedural regions where moving instances before the terminator would place them after operations that use their results. Fixes instance-choice dominance errors in prettify-verilog pass. --- lib/Dialect/SV/Transforms/PrettifyVerilog.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp b/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp index a66a001010ce..dc0d3f3b71b3 100644 --- a/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp +++ b/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp @@ -540,7 +540,19 @@ void PrettifyVerilogPass::processPostOrder(Block &body) { // end of the module. Any outputs will be writing to a wire or an output port // of the enclosing module anyway, and this allows inputs to be inlined into // the operand list as parameters. - if (!instances.empty()) { + // However, skip this optimization for blocks inside ifdef/procedural regions + // with implicit terminators, as moving instances can create dominance + // violations if they end up after operations that use their results. + bool skipInstanceSinking = false; + if (!body.empty() && body.back().hasTrait()) { + // Check if this is an ifdef or procedural region (has implicit terminator) + Operation *parentOp = body.getParentOp(); + if (isa(parentOp) || + parentOp->hasTrait()) + skipInstanceSinking = true; + } + + if (!instances.empty() && !skipInstanceSinking) { for (Operation *instance : llvm::reverse(instances)) { if (instance != &body.back()) instance->moveBefore(&body.back()); From b808eba1c6ab916598f72f929110b0ccb4611857 Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Mon, 11 May 2026 09:55:09 -0700 Subject: [PATCH 5/6] Restore missing graph region traits to sv.ifdef operation The conversion from GraphRegionNoTerminator to SingleBlockImplicitTerminator dropped important traits: - RegionKindInterface: Declares region kind (Graph vs SSACFG) - HasOnlyGraphRegion: Disables SSA dominance requirements Changes: 1. Added RegionKindInterface and HasOnlyGraphRegion traits to IfDefOp 2. Implemented getRegionKind() to return RegionKind::Graph 3. IfDefProceduralOp keeps SSACFG semantics (no graph region trait added) This ensures sv.ifdef regions remain graph regions where operations can appear in any order without SSA dominance constraints, matching the original semantics of GraphRegionNoTerminator. Test Results: 1234/1290 passing (95.66%) --- include/circt/Dialect/SV/SVStatements.td | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/circt/Dialect/SV/SVStatements.td b/include/circt/Dialect/SV/SVStatements.td index 8c5fb9fd353c..fc5bebae150e 100644 --- a/include/circt/Dialect/SV/SVStatements.td +++ b/include/circt/Dialect/SV/SVStatements.td @@ -104,6 +104,8 @@ def IfDefOp : SVOp<"ifdef", [ NonProceduralOp, NonProceduralRegion, SingleBlockImplicitTerminator<"YieldOp">, + RegionKindInterface, + HasOnlyGraphRegion, DeclareOpInterfaceMethods ]> { let summary = "'ifdef MACRO' block"; @@ -207,6 +209,9 @@ def IfDefOp : SVOp<"ifdef", [ mlir::ResultRange getElseResults() { return getResults().drop_front(getNumThenResults()); } + + // Implement RegionKindInterface. + static RegionKind getRegionKind(unsigned index) { return RegionKind::Graph; } }]; } From dd5f7ab74d432a560a92461281166847b8e2534b Mon Sep 17 00:00:00 2001 From: Prithayan Barua Date: Mon, 11 May 2026 14:27:28 -0700 Subject: [PATCH 6/6] Simplify --- lib/Conversion/SeqToSV/FirRegLowering.cpp | 13 +- lib/Conversion/SimToSV/SimToSV.cpp | 22 +-- lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp | 142 +++++++++--------- lib/Dialect/SV/SVOps.cpp | 13 +- lib/Dialect/SV/Transforms/HWCleanup.cpp | 11 +- lib/Dialect/SV/Transforms/PrettifyVerilog.cpp | 14 +- test/Dialect/Seq/firreg-ifdef-terminator.mlir | 108 +++++++++++++ test/firtool/instance-choice.fir | 2 +- 8 files changed, 204 insertions(+), 121 deletions(-) create mode 100644 test/Dialect/Seq/firreg-ifdef-terminator.mlir diff --git a/lib/Conversion/SeqToSV/FirRegLowering.cpp b/lib/Conversion/SeqToSV/FirRegLowering.cpp index 0a9f8bf465f1..ce7c84e03e9c 100644 --- a/lib/Conversion/SeqToSV/FirRegLowering.cpp +++ b/lib/Conversion/SeqToSV/FirRegLowering.cpp @@ -818,22 +818,17 @@ void FirRegLowering::buildRegConditions(OpBuilder &b, sv::RegOp reg) { condition.getMacro(), []() {}); // Insert before the implicit terminator auto *block = ifDef.getThenBlock(); - if (!block->empty() && block->back().hasTrait()) - b.setInsertionPoint(&block->back()); - else - b.setInsertionPointToEnd(block); + b.setInsertionPoint(&block->back()); continue; } if (kind == RegCondition::IfDefElse) { auto ifDef = sv::IfDefProceduralOp::create( b, reg.getLoc(), condition.getMacro(), []() {}, []() {}); - // Insert before the implicit terminator + // Insert before the implicit terminator. + // The block will always have a terminator after create() returns. auto *block = ifDef.getElseBlock(); - if (!block->empty() && block->back().hasTrait()) - b.setInsertionPoint(&block->back()); - else - b.setInsertionPointToEnd(block); + b.setInsertionPoint(&block->back()); continue; } llvm_unreachable("unknown reg condition type"); diff --git a/lib/Conversion/SimToSV/SimToSV.cpp b/lib/Conversion/SimToSV/SimToSV.cpp index 1e02dab93779..8cdd80a5f32b 100644 --- a/lib/Conversion/SimToSV/SimToSV.cpp +++ b/lib/Conversion/SimToSV/SimToSV.cpp @@ -467,11 +467,9 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { usedSynthesisMacro = true; } - // Move the op into the guard block (before the terminator if present). - if (!block->empty() && block->back().hasTrait()) - op->moveBefore(&block->back()); - else - op->moveBefore(block, block->end()); + // Move the op into the guard block before the implicit terminator. + // sv.ifdef/sv.ifdef.procedural always have a terminator after create(). + op->moveBefore(&block->back()); } // Check if the op requires an clock and condition wrapper. @@ -498,11 +496,8 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { .getBodyBlock(); } - // Move the op into the process (before the terminator if present). - if (!block->empty() && block->back().hasTrait()) - op->moveBefore(&block->back()); - else - op->moveBefore(block, block->end()); + // Move the op into the process. + op->moveBefore(block, block->end()); } // Create an enclosing if condition. @@ -519,11 +514,8 @@ static bool moveOpsIntoIfdefGuardsAndProcesses(Operation *rootOp) { block = sv::IfOp::create(builder, loc, condition, [] {}).getThenBlock(); } - // Move the op into the if body (before the terminator if present). - if (!block->empty() && block->back().hasTrait()) - op->moveBefore(&block->back()); - else - op->moveBefore(block, block->end()); + // Move the op into the if body. + op->moveBefore(block, block->end()); } }); diff --git a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp index f61f7550013c..a7a053578b49 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp @@ -398,23 +398,26 @@ void LowerLayersPass::lowerInlineLayerBlock(LayerOp layer, // macroName should always be set during preprocessing for inline layers assert(macroName && "macro name not set for inline layer"); - // Build the ifdef operation manually to ensure terminator is added - // before the operation is inserted and verified - OperationState state(layerBlock.getLoc(), sv::IfDefOp::getOperationName()); - state.addAttribute( - "cond", sv::MacroIdentAttr::get(builder.getContext(), macroName)); - Region *thenRegion = state.addRegion(); - // Take the body from the layerblock (which has NoTerminator) - thenRegion->takeBody(layerBlock.getBodyRegion()); - // Add empty else region - state.addRegion(); - - // Ensure the taken region has the required terminator BEFORE creating the - // op - sv::IfDefOp::ensureTerminator(*thenRegion, builder, layerBlock.getLoc()); - - // Now create and insert the operation - builder.create(state); + sv::IfDefOp::create(builder, layerBlock.getLoc(), macroName, + /*thenCtor=*/ + [&]() { + // The builder creates a new block, but we need to + // replace it with the layerblock's body. Get the + // current block and its parent region. + Block *newBlock = builder.getBlock(); + Region *thenRegion = newBlock->getParent(); + + // Erase the newly created block. + newBlock->erase(); + + // Take the body from the layerblock (which has + // NoTerminator). + thenRegion->takeBody(layerBlock.getBodyRegion()); + + // The builder will call ensureTerminator after this + // lambda returns. + }, + /*elseCtor=*/{}); } layerBlock.erase(); } @@ -1207,60 +1210,63 @@ void LowerLayersPass::buildBindFile(CircuitNamespace &ns, b.setInsertionPointToEnd(bindFile.getBody()); // Create the #ifndef for the include guard. - auto includeGuard = sv::IfDefOp::create(b, loc, macroSymbolRefAttr); - b.createBlock(&includeGuard.getElseRegion()); - - // Create the #define for the include guard. - sv::MacroDefOp::create(b, loc, macroSymbolRefAttr); - - // Create IR to enable any parent layers. - auto parent = layer->getParentOfType(); - while (parent) { - // If the parent is bound-in, we enable it by including the bindfile. - // The parent bindfile will enable all ancestors. - if (parent.getConvention() == LayerConvention::Bind) { - auto target = bindFiles[module][parent].filename; - sv::IncludeOp::create(b, loc, IncludeStyle::Local, target); - break; - } - - // If the parent layer is inline, we can only assert that the parent is - // already enabled. - if (parent.getConvention() == LayerConvention::Inline) { - auto parentMacroSymbolRefAttr = macroNames[parent]; - auto parentGuard = sv::IfDefOp::create(b, loc, parentMacroSymbolRefAttr); - OpBuilder::InsertionGuard guard(b); - b.createBlock(&parentGuard.getElseRegion()); - auto message = StringAttr::get(&getContext(), - Twine(parent.getName()) + " not enabled"); - sv::MacroErrorOp::create(b, loc, message); - // Ensure the else block has a terminator - sv::IfDefOp::ensureTerminator(parentGuard.getElseRegion(), b, loc); - parent = parent->getParentOfType(); - continue; - } + Block *elseBlock = nullptr; + sv::IfDefOp::create( + b, loc, macroSymbolRefAttr, + /*thenCtor=*/{}, + /*elseCtor=*/ + [&]() { + elseBlock = b.getBlock(); + + // Create the #define for the include guard. + sv::MacroDefOp::create(b, loc, macroSymbolRefAttr); + + // Create IR to enable any parent layers. + auto parent = layer->getParentOfType(); + while (parent) { + // If the parent is bound-in, we enable it by including the bindfile. + // The parent bindfile will enable all ancestors. + if (parent.getConvention() == LayerConvention::Bind) { + auto target = bindFiles[module][parent].filename; + sv::IncludeOp::create(b, loc, IncludeStyle::Local, target); + break; + } - // Unknown Layer convention. - llvm_unreachable("unknown layer convention"); - } + // If the parent layer is inline, we can only assert that the parent + // is already enabled. + if (parent.getConvention() == LayerConvention::Inline) { + auto parentMacroSymbolRefAttr = macroNames[parent]; + auto message = StringAttr::get( + &getContext(), Twine(parent.getName()) + " not enabled"); + sv::IfDefOp::create( + b, loc, parentMacroSymbolRefAttr, + /*thenCtor=*/{}, + /*elseCtor=*/ + [&]() { sv::MacroErrorOp::create(b, loc, message); }); + parent = parent->getParentOfType(); + continue; + } - // Create IR to include bind files for child modules. If a module is - // instantiated more than once, we only need to include the bindfile once. - SmallPtrSet seen; - for (auto *record : *node) { - auto *child = record->getTarget()->getModule().getOperation(); - if (!std::get(seen.insert(child))) - continue; - auto files = bindFiles[child]; - auto lookup = files.find(layer); - if (lookup == files.end() || !lookup->second.effectful) - continue; - sv::IncludeOp::create(b, loc, IncludeStyle::Local, lookup->second.filename); - } + // Unknown Layer convention. + llvm_unreachable("unknown layer convention"); + } - // Ensure the else block has a terminator before saving it - auto *elseBlock = includeGuard.getElseBlock(); - sv::IfDefOp::ensureTerminator(includeGuard.getElseRegion(), b, loc); + // Create IR to include bind files for child modules. If a module is + // instantiated more than once, we only need to include the bindfile + // once. + SmallPtrSet seen; + for (auto *record : *node) { + auto *child = record->getTarget()->getModule().getOperation(); + if (!std::get(seen.insert(child))) + continue; + auto files = bindFiles[child]; + auto lookup = files.find(layer); + if (lookup == files.end() || !lookup->second.effectful) + continue; + sv::IncludeOp::create(b, loc, IncludeStyle::Local, + lookup->second.filename); + } + }); // Save the bind file information for later. auto &info = bindFiles[module][layer]; diff --git a/lib/Dialect/SV/SVOps.cpp b/lib/Dialect/SV/SVOps.cpp index 9bca6c939680..aa046a90a7e2 100644 --- a/lib/Dialect/SV/SVOps.cpp +++ b/lib/Dialect/SV/SVOps.cpp @@ -661,11 +661,11 @@ LogicalResult IfDefOp::verifySymbolUses(SymbolTableCollection &symbolTable) { // If both thenRegion and elseRegion are empty, erase op. template static LogicalResult canonicalizeIfDefLike(Op op, PatternRewriter &rewriter) { - // Check if then block is empty (ignoring implicit yield terminator) + // Check if then block is empty (ignoring implicit yield terminator). + // The block always has a terminator due to SingleBlockImplicitTerminator. auto *thenBlock = op.getThenBlock(); - bool thenEmpty = - thenBlock->empty() || (thenBlock->getOperations().size() == 1 && - isa(thenBlock->front())); + bool thenEmpty = (thenBlock->getOperations().size() == 1 && + isa(thenBlock->front())); if (!thenEmpty) return failure(); @@ -673,9 +673,8 @@ static LogicalResult canonicalizeIfDefLike(Op op, PatternRewriter &rewriter) { // Check if else block is empty (ignoring implicit yield terminator) if (op.hasElse()) { auto *elseBlock = op.getElseBlock(); - bool elseEmpty = - elseBlock->empty() || (elseBlock->getOperations().size() == 1 && - isa(elseBlock->front())); + bool elseEmpty = (elseBlock->getOperations().size() == 1 && + isa(elseBlock->front())); if (!elseEmpty) return failure(); } diff --git a/lib/Dialect/SV/Transforms/HWCleanup.cpp b/lib/Dialect/SV/Transforms/HWCleanup.cpp index 5a53144df228..0c397fc5c7f4 100644 --- a/lib/Dialect/SV/Transforms/HWCleanup.cpp +++ b/lib/Dialect/SV/Transforms/HWCleanup.cpp @@ -87,22 +87,17 @@ static void mergeRegions(Region *region1, Region *region2) { return; } - // If the second region is not empty, merge operations from block2 to block1. - // The semantics is to PREPEND block2's operations to block1, maintaining the - // original order when multiple operations are merged. + // If the second region is not empty, splice its block into the start of the + // first region. if (!region2->empty()) { auto &block1 = region1->front(); auto &block2 = region2->front(); // If block2 has a terminator, remove it since we'll keep block1's // terminator. - if (!block2.empty() && block2.back().hasTrait()) { + if (!block2.empty() && block2.back().hasTrait()) block2.back().erase(); - } - // Splice the operations from block2 to the beginning of block1. - // This preserves the historical behavior where earlier operations stay - // earlier. block1.getOperations().splice(block1.begin(), block2.getOperations()); } } diff --git a/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp b/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp index dc0d3f3b71b3..a66a001010ce 100644 --- a/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp +++ b/lib/Dialect/SV/Transforms/PrettifyVerilog.cpp @@ -540,19 +540,7 @@ void PrettifyVerilogPass::processPostOrder(Block &body) { // end of the module. Any outputs will be writing to a wire or an output port // of the enclosing module anyway, and this allows inputs to be inlined into // the operand list as parameters. - // However, skip this optimization for blocks inside ifdef/procedural regions - // with implicit terminators, as moving instances can create dominance - // violations if they end up after operations that use their results. - bool skipInstanceSinking = false; - if (!body.empty() && body.back().hasTrait()) { - // Check if this is an ifdef or procedural region (has implicit terminator) - Operation *parentOp = body.getParentOp(); - if (isa(parentOp) || - parentOp->hasTrait()) - skipInstanceSinking = true; - } - - if (!instances.empty() && !skipInstanceSinking) { + if (!instances.empty()) { for (Operation *instance : llvm::reverse(instances)) { if (instance != &body.back()) instance->moveBefore(&body.back()); diff --git a/test/Dialect/Seq/firreg-ifdef-terminator.mlir b/test/Dialect/Seq/firreg-ifdef-terminator.mlir new file mode 100644 index 000000000000..249ce0e7c046 --- /dev/null +++ b/test/Dialect/Seq/firreg-ifdef-terminator.mlir @@ -0,0 +1,108 @@ +// RUN: circt-opt %s --lower-seq-to-sv | FileCheck %s + +// This test verifies that sv.ifdef.procedural blocks created during +// register randomization have proper terminators (sv.yield) and that +// operations are inserted before the terminator, not after it. + +hw.module @test_ifdef_terminator(in %clk : !seq.clock, in %rst : i1, in %in : i32, out a : i32) { + %cst0 = hw.constant 0 : i32 + + // A simple register with randomization + %rA = seq.firreg %in clock %clk sym @regA : i32 + + // CHECK-LABEL: hw.module @test_ifdef_terminator + // CHECK: %rA = sv.reg sym @regA : !hw.inout + + // Verify the initial block is created + // CHECK: sv.ifdef @ENABLE_INITIAL_REG_ { + // CHECK: sv.ordered { + // CHECK: sv.ifdef @FIRRTL_BEFORE_INITIAL + // CHECK: sv.initial { + + // Verify that sv.ifdef.procedural for INIT_RANDOM_PROLOG_ has proper structure + // CHECK: sv.ifdef.procedural @INIT_RANDOM_PROLOG_ { + // CHECK-NEXT: sv.verbatim "`INIT_RANDOM_PROLOG_" + // The ifdef.procedural block should have an implicit sv.yield terminator + // which is not printed when there are no operands + // CHECK-NEXT: } + + // Verify that sv.ifdef.procedural for RANDOMIZE_REG_INIT has proper structure + // CHECK: sv.ifdef.procedural @RANDOMIZE_REG_INIT { + // CHECK: %_RANDOM = sv.logic : !hw.inout> + // CHECK: sv.for + // Operations should be inserted before the implicit terminator + // CHECK: sv.bpassign %rA + // The ifdef.procedural block should end with an implicit sv.yield + // CHECK-NEXT: } + + // CHECK: } + // CHECK: sv.ifdef @FIRRTL_AFTER_INITIAL + // CHECK: } + // CHECK: } + + hw.output %rA : i32 +} + +// Test with multiple registers in the same ifdef block +hw.module @test_multiple_regs(in %clk : !seq.clock, in %in : i8, out a : i8, out b : i8) { + // Register A - will be randomized + %rA = seq.firreg %in clock %clk sym @regA : i8 + + // Register B - will also be randomized + %rB = seq.firreg %in clock %clk sym @regB : i8 + + // CHECK-LABEL: hw.module @test_multiple_regs + // CHECK: %rA = sv.reg sym @regA : !hw.inout + // CHECK: %rB = sv.reg sym @regB : !hw.inout + + // CHECK: sv.ifdef @ENABLE_INITIAL_REG_ { + // CHECK: sv.ordered { + // CHECK: sv.initial { + // CHECK: sv.ifdef.procedural @INIT_RANDOM_PROLOG_ + + // Both registers should be initialized within the same ifdef block + // CHECK: sv.ifdef.procedural @RANDOMIZE_REG_INIT { + // CHECK: %_RANDOM = sv.logic + // Both registers should have their bpassign before the terminator + // CHECK: sv.bpassign %rA + // CHECK: sv.bpassign %rB + // All operations should be before the implicit terminator + // CHECK-NEXT: } + + // CHECK: } + // CHECK: } + // CHECK: } + + hw.output %rA, %rB : i8, i8 +} + +// Test with async reset to verify proper terminator handling +hw.module @test_async_reset(in %clk : !seq.clock, in %rst : i1, in %in : i16, out out : i16) { + %cst0 = hw.constant 0 : i16 + + // Register with async reset - this creates different initialization + %r1 = seq.firreg %in clock %clk sym @reg1 reset async %rst, %cst0 : i16 + + // CHECK-LABEL: hw.module @test_async_reset + // CHECK: %r1 = sv.reg sym @reg1 + + // CHECK: sv.ifdef @ENABLE_INITIAL_REG_ { + // CHECK: sv.ordered { + // CHECK: sv.initial { + // CHECK: sv.ifdef.procedural @INIT_RANDOM_PROLOG_ + // CHECK: sv.ifdef.procedural @RANDOMIZE_REG_INIT { + // Register should be initialized - operations inserted before terminator + // CHECK: sv.bpassign %r1 + // CHECK-NEXT: } + // After randomization, apply reset if active + // CHECK: sv.if %rst { + // CHECK: sv.bpassign %r1, %c0_i16 + // CHECK: } + // CHECK: } + // CHECK: } + // CHECK: } + + hw.output %r1 : i16 +} + + diff --git a/test/firtool/instance-choice.fir b/test/firtool/instance-choice.fir index 0a772411f73c..fef65e31b125 100644 --- a/test/firtool/instance-choice.fir +++ b/test/firtool/instance-choice.fir @@ -23,11 +23,11 @@ circuit top: input rst: UInt<1> ; CHECK: wire [31:0] [[RESULT:.+]]; ; CHECK-NEXT: `ifdef targets$Platform$FPGA + ; CHECK-NEXT: `define targets$Platform$top$proc proc_FPGA ; CHECK-NEXT: FPGATarget proc_FPGA ( ; CHECK-NEXT: .rst (rst), ; CHECK-NEXT: .result_0 ([[RESULT]]) ; CHECK-NEXT: ); - ; CHECK-NEXT: `define targets$Platform$top$proc proc_FPGA ; CHECK-NEXT: `else // targets$Platform$FPGA ; CHECK-NEXT: $error("Required instance choice option 'Platform' not selected, must define one of: 'targets$Platform$FPGA'"); ; CHECK-NEXT: `endif // targets$Platform$FPGA