Add sv.verbatim.source and sv.verbatim.module ops for inline black box extmodules #9227
Conversation
| def SVVerbatimSourceOp | ||
| : SVOp<"verbatim.source", [Symbol, HasParent<"mlir::ModuleOp">, | ||
| DeclareOpInterfaceMethods<SymbolUserOpInterface>]> { |
There was a problem hiding this comment.
Nit: this seems weird to put this in SVTypeDecl.td. Usually these would go in an SVStructure.td.
There was a problem hiding this comment.
I found no SVStructure.td file and thought it would be most appropriate alongside the existing op definitions.
There was a problem hiding this comment.
Extreme nit: This is over 80 column. 😉
| // Duplicated extmodule definitions to the same output file. This can | ||
| // happen when generators produce definitions of an inline black box | ||
| // under each of multiple deduping scopes. |
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
LGTM
Ping me when you want a merge if you haven't, yet, gained commit access.
| def SVVerbatimSourceOp | ||
| : SVOp<"verbatim.source", [Symbol, HasParent<"mlir::ModuleOp">, | ||
| DeclareOpInterfaceMethods<SymbolUserOpInterface>]> { |
There was a problem hiding this comment.
Extreme nit: This is over 80 column. 😉
| The "sv.verbatim.source" operation represents verbatim verilog definition for | ||
| a module which may have parameters. Concrete parametrizations and their ports | ||
| are represented by a `hw.module.extern`. |
| portAttrs.push_back(port.attrs ? port.attrs : $_builder.getDictionaryAttr({})); | ||
| portLocs.push_back(port.loc ? static_cast<Location>(port.loc) : $_builder.getUnknownLoc()); |
| auto it = verbatimSourcesByFileName.find(fileName); | ||
| return it != verbatimSourcesByFileName.end() ? it->second : nullptr; |
There was a problem hiding this comment.
Does this work instead?
| auto it = verbatimSourcesByFileName.find(fileName); | |
| return it != verbatimSourcesByFileName.end() ? it->second : nullptr; | |
| return verbatimSourcesByFileName.return(fileName); |
Ditto throughout.
| void registerVerbatimSource(StringRef fileName, | ||
| sv::SVVerbatimSourceOp verbatimOp) { | ||
| llvm::sys::SmartScopedLock<true> lock(verbatimSourcesMutex); | ||
| verbatimSourcesByFileName[fileName] = verbatimOp; |
There was a problem hiding this comment.
This could overwrite. Is that fine?
Also, is it legal for verbatimOp to be null here?
The latter would usually be handled with an assert if it is some assumption that isn't supposed to be violated generally.
| StringRef verilogName; | ||
| if (auto defName = oldModule.getDefname()) | ||
| verilogName = defName.value(); | ||
| else | ||
| verilogName = oldModule.getName(); |
| /*builder=*/builder, | ||
| /*location=*/oldModule.getLoc(), | ||
| /*name=*/builder.getStringAttr(oldModule.getName()), | ||
| /*ports=*/ports, | ||
| /*source=*/FlatSymbolRefAttr::get(verbatimSource), | ||
| /*parameters=*/parameters ? parameters : builder.getArrayAttr({}), | ||
| /*verilogName=*/verilogName.empty() ? StringAttr{} | ||
| : builder.getStringAttr(verilogName)); |
There was a problem hiding this comment.
This is nicely readable to me! Thanks!
| llvm::MapVector<Operation *, SmallVector<StringAttr>> | ||
| verbatimExtmoduleToFiles; |
There was a problem hiding this comment.
Consider llvm::MapVector<Operation *, SmallVector<StringRef>>.
This is a micro-optimization, but should be lower overhead so long as you can guarantee that the backing string will not go out of scope!. I think this is safe as the backing thing here should be a StringAttr for some Operation which can't go out of scope. (Famous last words...)
| if parameters is not None: | ||
| attributes["parameters"] = parameters | ||
| else: | ||
| attributes["parameters"] = ArrayAttr.get([]) |
There was a problem hiding this comment.
Nit: this could be pre-allocated as [] and then appended to.
3 participants
A second attempt of #9131. Changes from the first PR were made in order to support parametrized black boxes. Because parameters can affect the port interface of a module, we need two ops:
sv.verbatim.sourceto represent the shared verbatim definition of a modulesv.verbatim.moduleto represent a particular parametrization of that module that can be instantiatedThe pipeline is largely the same:
BlackBoxInlineAnnoandBlackBoxPathAnnoare converted toVerbatimBlackBoxAnnoby BlackBoxReader for consumption by LowerToHW.firrtl.extmodules with aVerbatimBlackBoxAnnoto asv.verbatim.source(only one per file) and asv.verbatim.module.