-
Notifications
You must be signed in to change notification settings - Fork 469
[FIRRTLToHW] Remove the SYNTHESIS guard in printf/flush lowering path #10526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
1299751
e7c2c5b
4367640
e760eda
f587125
97326ff
92cd9c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
nanjo712 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,10 +62,15 @@ void sv::emitFileDescriptorRuntime(Operation *fileScopeOp, | |
| auto getterSymName = ::getFileDescriptorGetterSymName(builder.getContext()); | ||
| auto fragmentSymName = | ||
| ::getFileDescriptorFragmentSymName(builder.getContext()); | ||
| auto macroSymName = builder.getStringAttr(kFileDescriptorMacroName); | ||
| bool fileDescriptorMacroNeedsCreation = false; | ||
| auto macroSymName = sv::resolveMacroSymName( | ||
| fileScopeOp, kFileDescriptorMacroName, fileDescriptorMacroNeedsCreation); | ||
| bool synthesisMacroNeedsCreation = false; | ||
| auto synthesisSymName = sv::resolveMacroSymName(fileScopeOp, "SYNTHESIS", | ||
| synthesisMacroNeedsCreation); | ||
| SymbolTable symbolTable(fileScopeOp); | ||
|
|
||
| auto emitGuard = [&](StringRef guard, llvm::function_ref<void(void)> body) { | ||
| auto emitGuard = [&](StringAttr guard, llvm::function_ref<void(void)> body) { | ||
| sv::IfDefOp::create( | ||
| builder, guard, [] {}, body); | ||
| }; | ||
|
|
@@ -95,15 +100,30 @@ void sv::emitFileDescriptorRuntime(Operation *fileScopeOp, | |
| symbolTable.insert(func); | ||
| } | ||
|
|
||
| if (!symbolTable.lookup(macroSymName)) | ||
| symbolTable.insert(sv::MacroDeclOp::create(builder, macroSymName)); | ||
| if (synthesisMacroNeedsCreation) { | ||
| auto verilogName = synthesisSymName.getValue() == "SYNTHESIS" | ||
| ? StringAttr{} | ||
| : builder.getStringAttr("SYNTHESIS"); | ||
| symbolTable.insert( | ||
| sv::MacroDeclOp::create(builder, builder.getLoc(), synthesisSymName, | ||
| /*args=*/ArrayAttr{}, verilogName)); | ||
|
nanjo712 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| if (fileDescriptorMacroNeedsCreation) { | ||
| auto verilogName = macroSymName.getValue() == kFileDescriptorMacroName | ||
| ? StringAttr{} | ||
| : builder.getStringAttr(kFileDescriptorMacroName); | ||
| symbolTable.insert( | ||
| sv::MacroDeclOp::create(builder, builder.getLoc(), macroSymName, | ||
| /*args=*/ArrayAttr{}, verilogName)); | ||
| } | ||
|
|
||
| if (symbolTable.lookup(fragmentSymName)) | ||
| return; | ||
|
|
||
| auto fragment = emit::FragmentOp::create(builder, fragmentSymName, [&] { | ||
| emitGuard("SYNTHESIS", [&]() { | ||
| emitGuard(kFileDescriptorMacroName, [&]() { | ||
| emitGuard(synthesisSymName, [&]() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we remove this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be more cautious about removing this. We could simply remove the synthesis guard for print operations, mainly because Chisel places these operations in a separate layer, which are then output to separate files. These separate files themselves have macro guard, and also it's easy to disable it by not including these separate files in the synthesis. As far as I know, there doesn't seem to be a similar mechanism for this part; it outputs separately to each file used, unlike print operations which are separated from the design code. We might be able to improve it further, but I think this might require another evaluation. |
||
| emitGuard(macroSymName, [&]() { | ||
| sv::VerbatimOp::create(builder, R"(// CIRCT Logging Library | ||
| package __circt_lib_logging; | ||
| class FileDescriptor; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| #include "circt/Dialect/HW/ModuleImplementation.h" | ||
| #include "circt/Dialect/SV/SVAttributes.h" | ||
| #include "circt/Support/CustomDirectiveImpl.h" | ||
| #include "circt/Support/Namespace.h" | ||
| #include "circt/Support/ProceduralRegionTrait.h" | ||
| #include "mlir/IR/Builders.h" | ||
| #include "mlir/IR/BuiltinTypes.h" | ||
|
|
@@ -56,6 +57,36 @@ bool sv::isExpression(Operation *op) { | |
| MacroRefExprOp, MacroRefExprSEOp>(op); | ||
| } | ||
|
|
||
| StringAttr sv::resolveMacroSymName(Operation *symbolTableOp, | ||
| StringRef verilogName, bool &created) { | ||
| assert(symbolTableOp && "expected symbol table op"); | ||
| assert(symbolTableOp->hasTrait<mlir::OpTrait::SymbolTable>() && | ||
| "expected symbolTableOp to define a symbol table"); | ||
| assert(symbolTableOp->getNumRegions() == 1 && | ||
| "expected single-region symbol table op"); | ||
| assert(!symbolTableOp->getRegion(0).empty() && | ||
| "expected non-empty symbol table region"); | ||
|
nanjo712 marked this conversation as resolved.
Outdated
|
||
|
|
||
| for (auto decl : | ||
| symbolTableOp->getRegion(0).front().getOps<sv::MacroDeclOp>()) { | ||
| if (decl.getMacroIdentifier() == verilogName) { | ||
| created = false; | ||
| return decl.getSymNameAttr(); | ||
| } | ||
| } | ||
|
|
||
| Namespace ns; | ||
| auto symbolNameId = StringAttr::get(symbolTableOp->getContext(), | ||
| SymbolTable::getSymbolAttrName()); | ||
| for (auto &op : symbolTableOp->getRegion(0).front()) { | ||
| if (auto symbol = op.getAttrOfType<StringAttr>(symbolNameId)) | ||
| ns.add(symbol.getValue()); | ||
| } | ||
|
|
||
| created = true; | ||
| return StringAttr::get(symbolTableOp->getContext(), ns.newName(verilogName)); | ||
| } | ||
|
Comment on lines
+70
to
+82
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand it existed before, but could you consider using SymbolTable::insert instead (as SymbolTable is MLIR proper)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may have misunderstood. I think |
||
|
|
||
| /// Returns the operation registered with the given symbol name with the regions | ||
| /// of 'symbolTableOp'. recurse through nested regions which don't contain the | ||
| /// symboltable trait. Returns nullptr if no valid symbol was found. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| add_subdirectory(Utils) | ||
|
|
||
| if(CIRCT_SLANG_FRONTEND_ENABLED) | ||
| add_subdirectory(ImportVerilog) | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| add_circt_unittest(CIRCTConversionUtilsTests | ||
| SVLoweringUtilsTest.cpp | ||
| ) | ||
|
|
||
| target_link_libraries(CIRCTConversionUtilsTests | ||
| PRIVATE | ||
| CIRCTEmit | ||
| CIRCTHW | ||
| CIRCTSV | ||
| CIRCTSVLoweringUtils | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: naming this
getOrCreatemay be clearer:Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't actually create anything; it just checks if a symbol already exists or generates a usable symbol name. Perhaps it could be replaced with
lookupOrGenerateMacroSymName?