diff --git a/include/circt/Dialect/HW/HW.td b/include/circt/Dialect/HW/HW.td index c2dc2f225ce2..fa9c2a8ade69 100644 --- a/include/circt/Dialect/HW/HW.td +++ b/include/circt/Dialect/HW/HW.td @@ -22,6 +22,7 @@ include "mlir/IR/RegionKindInterface.td" include "circt/Dialect/HW/HWDialect.td" include "circt/Dialect/HW/HWAttributes.td" +include "circt/Dialect/HW/HWAttributesNaming.td" include "circt/Dialect/HW/HWTypesImpl.td" include "circt/Dialect/HW/HWTypes.td" diff --git a/include/circt/Dialect/HW/HWAttributes.h b/include/circt/Dialect/HW/HWAttributes.h index 1cfe54d11d44..cbbbaa33467c 100644 --- a/include/circt/Dialect/HW/HWAttributes.h +++ b/include/circt/Dialect/HW/HWAttributes.h @@ -16,6 +16,12 @@ namespace circt { namespace hw { class PEOAttr; enum class PEO : uint32_t; + +// Eventually move this to an op trait +struct InnerName { + static llvm::StringRef getInnerNameAttrName() { return "inner_sym"; } +}; + } // namespace hw } // namespace circt diff --git a/include/circt/Dialect/HW/HWAttributesNaming.td b/include/circt/Dialect/HW/HWAttributesNaming.td new file mode 100644 index 000000000000..fd42284681c8 --- /dev/null +++ b/include/circt/Dialect/HW/HWAttributesNaming.td @@ -0,0 +1,25 @@ +//===- HWAttributesNaming.td - Attributes for HW dialect ---*- tablegen -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines HW dialect attributes used in other dialects. +// +//===----------------------------------------------------------------------===// + +def InnerRefAttr : AttrDef { + let summary = "Refer to a name inside a module"; + let description = [{ + This works like a symbol reference, but to a name inside a module. + }]; + let mnemonic = "innerNameRef"; + let parameters = (ins "::mlir::StringAttr":$module, "::mlir::StringAttr":$name); + let builders = [ + AttrBuilderWithInferredContext<(ins "::mlir::StringAttr":$module, "::mlir::StringAttr":$name),[{ + return get(module.getContext(), module, name); + }]>, + ]; +} diff --git a/include/circt/Dialect/HW/HWOps.h b/include/circt/Dialect/HW/HWOps.h index 5f8cdd7a450e..f0ad6d4b7fa6 100644 --- a/include/circt/Dialect/HW/HWOps.h +++ b/include/circt/Dialect/HW/HWOps.h @@ -22,6 +22,7 @@ #include "mlir/IR/SymbolTable.h" #include "mlir/Interfaces/ControlFlowInterfaces.h" #include "mlir/Interfaces/SideEffectInterfaces.h" +#include "llvm/ADT/StringExtras.h" namespace circt { namespace hw { @@ -148,23 +149,62 @@ LogicalResult checkParameterInContext(Attribute value, Operation *module, /// "freeze" method transitions between the two states. class SymbolCache { public: + class Item { + public: + Item(Operation *op) : op(op), port(~0ULL) {} + Item(Operation *op, size_t port) : op(op), port(port) {} + bool hasPort() const { return port != ~0ULL; } + Operation *getOp() const { return op; } + size_t getPort() const { return port; } + + private: + Operation *op; + size_t port; + }; + /// In the building phase, add symbols. void addDefinition(StringAttr symbol, Operation *op) { assert(!isFrozen && "cannot mutate a frozen cache"); - symbolCache[symbol.getValue()] = op; + symbolCache.try_emplace(symbol.getValue(), op, ~0UL); + } + + // Add inner names, which might be ports + void addDefinition(StringAttr symbol, StringRef name, Operation *op, + size_t port = ~0UL) { + assert(!isFrozen && "cannot mutate a frozen cache"); + auto key = mkInnerKey(symbol.getValue(), name); + symbolCache.try_emplace(StringRef(key.data(), key.size()), op, port); } /// Mark the cache as frozen, which allows it to be shared across threads. void freeze() { isFrozen = true; } - Operation *getDefinition(StringAttr symbolName) const { + Operation *getDefinition(StringRef symbol) const { assert(isFrozen && "cannot read from this cache until it is frozen"); - auto it = symbolCache.find(symbolName.getValue()); - return it != symbolCache.end() ? it->second : nullptr; + auto it = symbolCache.find(symbol); + if (it == symbolCache.end()) + return nullptr; + assert(!it->second.hasPort() && "Module names should never be ports"); + return it->second.getOp(); + } + + Operation *getDefinition(StringAttr symbol) const { + return getDefinition(symbol.getValue()); } Operation *getDefinition(FlatSymbolRefAttr symbol) const { - return getDefinition(symbol.getAttr()); + return getDefinition(symbol.getValue()); + } + + Item getDefinition(StringRef symbol, StringRef name) const { + assert(isFrozen && "cannot read from this cache until it is frozen"); + auto key = mkInnerKey(symbol, name); + auto it = symbolCache.find(StringRef(key.data(), key.size())); + return it == symbolCache.end() ? Item{nullptr, ~0UL} : it->second; + } + + Item getDefinition(StringAttr symbol, StringAttr name) const { + return getDefinition(symbol.getValue(), name.getValue()); } private: @@ -173,9 +213,22 @@ class SymbolCache { /// This stores a lookup table from symbol attribute to the operation /// (hw.module, hw.instance, etc) that defines it. /// TODO: It is super annoying that symbols are *defined* as StringAttr, but - /// are then referenced as FlatSymbolRefAttr. Why can't we have nice pointer - /// uniqued things?? :-( - llvm::StringMap symbolCache; + /// are then referenced as FlatSymbolRefAttr. Why can't we have nice + /// pointer uniqued things?? :-( + llvm::StringMap symbolCache; + + // Construct a string key with embedded null. StringMapImpl::FindKey uses + // explicit lengths and stores keylength, rather than relying on null + // characters. + SmallVector mkInnerKey(StringRef mod, StringRef name) const { + assert(!mod.contains(0) && !name.contains(0) && + "Null character in identifier"); + SmallVector key; + key.append(mod.begin(), mod.end()); + key.push_back(0); + key.append(name.begin(), name.end()); + return key; + } }; } // namespace hw diff --git a/include/circt/Dialect/HW/HWStructure.td b/include/circt/Dialect/HW/HWStructure.td index 5f6d2c7f114a..6f1cbba1d4e3 100644 --- a/include/circt/Dialect/HW/HWStructure.td +++ b/include/circt/Dialect/HW/HWStructure.td @@ -325,7 +325,7 @@ def HWModuleGeneratedOp : HWOp<"module.generated", let verifier = "return ::verify$cppClass(*this);"; } -def InstanceOp : HWOp<"instance", [Symbol, +def InstanceOp : HWOp<"instance", [ DeclareOpInterfaceMethods, DeclareOpInterfaceMethods]> { let summary = "Create an instance of a module"; @@ -343,7 +343,7 @@ def InstanceOp : HWOp<"instance", [Symbol, Variadic:$inputs, StrArrayAttr:$argNames, StrArrayAttr:$resultNames, ParamDeclArrayAttr:$parameters, - OptionalAttr:$sym_name); + OptionalAttr:$inner_sym); let results = (outs Variadic); let builders = [ diff --git a/include/circt/Dialect/HW/HWTypesImpl.td b/include/circt/Dialect/HW/HWTypesImpl.td index 68822f6ca315..fa9a3ed4ee2c 100644 --- a/include/circt/Dialect/HW/HWTypesImpl.td +++ b/include/circt/Dialect/HW/HWTypesImpl.td @@ -174,4 +174,3 @@ def TypeAliasType : HWType<"TypeAlias"> { TypedeclOp getTypeDecl(const SymbolCache &cache); }]; } - diff --git a/include/circt/Dialect/SV/SV.td b/include/circt/Dialect/SV/SV.td index d5ba1298e2af..d61f4b666d4d 100644 --- a/include/circt/Dialect/SV/SV.td +++ b/include/circt/Dialect/SV/SV.td @@ -51,6 +51,7 @@ def NonProceduralOp : NativeOpTrait<"NonProceduralOp"> { } include "circt/Dialect/HW/HWTypes.td" +include "circt/Dialect/HW/HWAttributesNaming.td" include "circt/Dialect/SV/SVTypes.td" include "circt/Dialect/SV/SVExpressions.td" include "circt/Dialect/SV/SVInOutOps.td" diff --git a/include/circt/Dialect/SV/SVExpressions.td b/include/circt/Dialect/SV/SVExpressions.td index a9c5c8891a95..0901be2b6e97 100644 --- a/include/circt/Dialect/SV/SVExpressions.td +++ b/include/circt/Dialect/SV/SVExpressions.td @@ -29,7 +29,7 @@ def VerbatimExprOp : SVOp<"verbatim.expr", [NoSideEffect, HasCustomSSAName]> { }]; let arguments = (ins StrAttr:$string, Variadic:$operands, - DefaultValuedAttr:$symbols); + DefaultValuedAttr:$symbols); let results = (outs AnySignlessIntegerOrInOutType:$result); let assemblyFormat = [{ $string (`(` $operands^ `)`)? @@ -65,7 +65,7 @@ def VerbatimExprSEOp : SVOp<"verbatim.expr.se", [HasCustomSSAName]> { }]; let arguments = (ins StrAttr:$string, Variadic:$operands, - DefaultValuedAttr:$symbols ); + DefaultValuedAttr:$symbols ); let results = (outs AnySignlessIntegerOrInOutType:$result); let assemblyFormat = [{ $string (`(` $operands^ `)`)? diff --git a/include/circt/Dialect/SV/SVInOutOps.td b/include/circt/Dialect/SV/SVInOutOps.td index 1207cc9882ca..c912e9a7b867 100644 --- a/include/circt/Dialect/SV/SVInOutOps.td +++ b/include/circt/Dialect/SV/SVInOutOps.td @@ -12,7 +12,7 @@ //===----------------------------------------------------------------------===// // Note that net declarations like 'wire' should not appear in an always block. -def WireOp : SVOp<"wire", [NonProceduralOp, Symbol, +def WireOp : SVOp<"wire", [NonProceduralOp, DeclareOpInterfaceMethods]> { let summary = "Define a new wire"; let description = [{ @@ -20,21 +20,21 @@ def WireOp : SVOp<"wire", [NonProceduralOp, Symbol, See SV Spec 6.7, pp97. }]; - let arguments = (ins StrAttr:$name, OptionalAttr:$sym_name); + let arguments = (ins StrAttr:$name, OptionalAttr:$inner_sym); let results = (outs InOutType:$result); let skipDefaultBuilders = 1; let builders = [ OpBuilder<(ins "::mlir::Type":$elementType, CArg<"StringAttr", "StringAttr()">:$name, - CArg<"StringAttr", "StringAttr()">:$sym_name)>, + CArg<"StringAttr", "StringAttr()">:$inner_sym)>, OpBuilder<(ins "::mlir::Type":$elementType, CArg<"StringRef">:$name), [{ return build($_builder, $_state, elementType, $_builder.getStringAttr(name)); }]> ]; - let assemblyFormat = [{ (`sym` $sym_name^)? custom(attr-dict) + let assemblyFormat = [{ (`sym` $inner_sym^)? custom(attr-dict) `:` type($result) }]; let hasCanonicalizeMethod = true; @@ -42,37 +42,29 @@ def WireOp : SVOp<"wire", [NonProceduralOp, Symbol, Type getElementType() { return result().getType().cast().getElementType(); } - - //===------------------------------------------------------------------===// - // SymbolOpInterface Methods - //===------------------------------------------------------------------===// - - /// An WireOp may optionally define a symbol. - bool isOptionalSymbol() { return true; } - }]; let verifier = [{ return ::verifyWireOp(*this); }]; } -def RegOp : SVOp<"reg", [Symbol, +def RegOp : SVOp<"reg", [ DeclareOpInterfaceMethods]> { let summary = "Define a new `reg` in SystemVerilog"; let description = [{ Declare a SystemVerilog Variable Declaration of 'reg' type. See SV Spec 6.8, pp100. }]; - let arguments = (ins StrAttr:$name, OptionalAttr:$sym_name); + let arguments = (ins StrAttr:$name, OptionalAttr:$inner_sym); let results = (outs InOutType:$result); let skipDefaultBuilders = 1; let builders = [ OpBuilder<(ins "::mlir::Type":$elementType, CArg<"StringAttr", "StringAttr()">:$name, - CArg<"StringAttr", "StringAttr()">:$sym_name)> + CArg<"StringAttr", "StringAttr()">:$inner_sym)> ]; // We handle the name in a custom way, so we use a customer parser/printer. - let assemblyFormat = [{ (`sym` $sym_name^)? custom(attr-dict) + let assemblyFormat = [{ (`sym` $inner_sym^)? custom(attr-dict) `:` type($result) }]; let hasCanonicalizeMethod = true; @@ -80,13 +72,6 @@ def RegOp : SVOp<"reg", [Symbol, Type getElementType() { return result().getType().cast().getElementType(); } - - //===------------------------------------------------------------------===// - // SymbolOpInterface Methods - //===------------------------------------------------------------------===// - - /// An RegOp may optionally define a symbol. - bool isOptionalSymbol() { return true; } }]; } diff --git a/include/circt/Dialect/SV/SVOps.h b/include/circt/Dialect/SV/SVOps.h index 9f2dffb07920..b82e922170e2 100644 --- a/include/circt/Dialect/SV/SVOps.h +++ b/include/circt/Dialect/SV/SVOps.h @@ -23,6 +23,7 @@ namespace circt { namespace hw { class InstanceOp; class SymbolCache; +class InnerRefAttr; } // namespace hw namespace sv { diff --git a/include/circt/Dialect/SV/SVStatements.td b/include/circt/Dialect/SV/SVStatements.td index 214b7cad3769..870589bb5a72 100644 --- a/include/circt/Dialect/SV/SVStatements.td +++ b/include/circt/Dialect/SV/SVStatements.td @@ -440,7 +440,7 @@ def VerbatimOp : SVOp<"verbatim"> { sv.verbatim allows operand substitutions with {{0}} syntax. }]; let arguments = (ins StrAttr:$string, Variadic:$operands, - DefaultValuedAttr:$symbols); + DefaultValuedAttr:$symbols); let assemblyFormat = "$string (`(` $operands^ `)` `:` type($operands))? attr-dict"; @@ -473,12 +473,14 @@ def BindOp : SVOp<"bind", []> { }]; let arguments = - (ins FlatSymbolRefAttr:$boundInstance, FlatSymbolRefAttr:$instanceModule); + (ins InnerRefAttr:$instance); let results = (outs); let verifier = "return ::verify$cppClass(*this);"; - let assemblyFormat = "$boundInstance `in` $instanceModule attr-dict"; - + let assemblyFormat = "$instance attr-dict"; + let builders = [ + OpBuilder<(ins "StringAttr":$mod, "StringAttr":$name)> + ]; let extraClassDeclaration = [{ /// Lookup the instance for the bind. This returns null on invalid IR. hw::InstanceOp getReferencedInstance(const hw::SymbolCache *cache = nullptr); diff --git a/include/circt/Dialect/SV/SVTypes.td b/include/circt/Dialect/SV/SVTypes.td index 3e059795abc9..730c3fb54a0a 100644 --- a/include/circt/Dialect/SV/SVTypes.td +++ b/include/circt/Dialect/SV/SVTypes.td @@ -44,7 +44,7 @@ class InOutElementConstraint inoutValue, value, "sv::getInOutElementType($_self)">; // This type is an array of FlatSymbolRefAttr. -//TODO: This should be moved to mlir, it does not depend on HW dialect. +// TODO: This should be moved to mlir, it does not depend on HW dialect. def SymRefArrayAttr: ArrayAttrBase< And<[ // Guarantee this is an ArrayAttr first @@ -57,6 +57,19 @@ def SymRefArrayAttr: ArrayAttrBase< let constBuilderCall = "$_builder.getArrayAttr($0)"; } +// Like a SymRefArrayAttr, but can also refer to names inside modules. +def NameRefArrayAttr: ArrayAttrBase< + And<[ + // Guarantee this is an ArrayAttr first + CPred<"$_self.isa<::mlir::ArrayAttr>()">, + // Guarantee all elements are symbols or name refs + CPred<"::llvm::all_of($_self.cast<::mlir::ArrayAttr>(), " + "[&](::mlir::Attribute attr) { return attr.isa<" + "::mlir::FlatSymbolRefAttr, ::hw::InnerRefAttr>();})">]>, + ""> { + let constBuilderCall = "$_builder.getArrayAttr($0)"; +} + def StringArrayAttr: ArrayAttrBase< And<[ // Guarantee this is an ArrayAttr first @@ -67,4 +80,5 @@ def StringArrayAttr: ArrayAttrBase< "::mlir::StringAttr>();})">]>, ""> { let constBuilderCall = "$_builder.getArrayAttr($0)"; -} \ No newline at end of file +} + diff --git a/lib/Conversion/ExportVerilog/ExportVerilog.cpp b/lib/Conversion/ExportVerilog/ExportVerilog.cpp index fb3f484d842c..90f499332501 100644 --- a/lib/Conversion/ExportVerilog/ExportVerilog.cpp +++ b/lib/Conversion/ExportVerilog/ExportVerilog.cpp @@ -442,6 +442,22 @@ struct ModuleNameManager { return getName(ValueOrOp(op)); } + StringRef getName(Operation *op, size_t port) { + // Module come in flavors and have ports, but don't have an opInterface in + // common to access the ports as blockvalues. Thus we have to enumerate + // modules here. + BlockArgument blockArg; + if (auto mod = dyn_cast(op)) + blockArg = mod.getArgument(port); + if (auto mod = dyn_cast(op)) + blockArg = mod.getArgument(port); + if (auto mod = dyn_cast(op)) + blockArg = mod.getArgument(port); + if (!blockArg) + llvm_unreachable("Unknown named thing with port"); + return getName(blockArg); + } + bool hasName(Value value) { return nameTable.count(ValueOrOp(value)); } bool hasName(Operation *op) { @@ -604,11 +620,40 @@ void EmitterBase::emitTextWithSubstitutions( // Perform operand substitions as we emit the line string. We turn {{42}} // into the value of operand 42. - SmallVector symOps; - for (auto sym : symAttrs) - if (auto symOp = - state.symbolCache.getDefinition(sym.cast())) - symOps.push_back(symOp); + SmallVector, 8> symOps; + auto namify = [&](SymbolCache::Item item) { + // Get the verilog name of the operation, add the name if not already + // done. + auto op = item.getOp(); + if (item.hasPort()) + return names.getName(op, item.getPort()); + // FIXME: is this really necessary? Shouldn't all referenced innernames + // have been named? + if (!names.hasName(op)) { + llvm::errs() << "ADL: @" << op->getAttrOfType("sym_name") + << "\n"; + op->dump(); + llvm::errs() << "**END\n"; + } + if (names.hasName(op)) + return names.getName(op); + StringRef symOpName = getSymOpName(op); + if (!symOpName.empty()) + return symOpName; + op->emitError("cannot get name for symbol"); + return StringRef(""); + }; + + for (auto sym : symAttrs) { + if (auto fsym = sym.dyn_cast()) + if (auto symOp = state.symbolCache.getDefinition(fsym)) + symOps.push_back(namify(symOp)); + if (auto isym = sym.dyn_cast()) { + auto symOp = + state.symbolCache.getDefinition(isym.getModule(), isym.getName()); + symOps.push_back(namify(symOp)); + } + } // Scan 'line' for a substitution, emitting any non-substitution prefix, // then the mentioned operand, chopping the relevant text off 'line' and @@ -655,22 +700,7 @@ void EmitterBase::emitTextWithSubstitutions( operandEmitter(op->getOperand(operandNo)); else if ((operandNo - op->getNumOperands()) < numSymOps) { unsigned symOpNum = operandNo - op->getNumOperands(); - Operation *symOp = symOps[symOpNum]; - // Get the verilog name of the operation, add the name if not already - // done. - if (names.hasName(symOp)) { - os << names.getName(symOp); - } else { - StringRef symOpName = getSymOpName(symOp); - if (!symOpName.empty()) { - os << symOpName; - } else { - std::string opStr; - llvm::raw_string_ostream tName(opStr); - tName << *symOp; - op->emitError("cannot get name for symbol: " + tName.str()); - } - } + os << symOps[symOpNum]; } else { emitError(op, "operand " + llvm::utostr(operandNo) + " isn't valid"); continue; @@ -3866,21 +3896,29 @@ struct SharedEmitterState { /// of an explicit output file attribute. void SharedEmitterState::gatherFiles(bool separateModules) { - /// Collect all the instance symbols from the specified module and add them - /// to the IRCache. Instances only exist at the top level of the module. - /// Also keep track of any modules that contain bind operations. These are - /// non-hierarchical references which we need to be careful about during - /// emission. + /// Collect all the inner names from the specified module and add them to the + /// IRCache. Declarations (named things) only exist at the top level of the + /// module. Also keep track of any modules that contain bind operations. + /// These are non-hierarchical references which we need to be careful about + /// during emission. auto collectInstanceSymbolsAndBinds = [&](HWModuleOp moduleOp) { for (Operation &op : *moduleOp.getBodyBlock()) { // Populate the symbolCache with all operations that can define a symbol. - if (auto symOp = dyn_cast(op)) - if (auto name = symOp.getNameAttr()) - symbolCache.addDefinition(name, symOp); + if (auto name = op.getAttrOfType( + hw::InnerName::getInnerNameAttrName())) + symbolCache.addDefinition(moduleOp.getNameAttr(), name.getValue(), &op); if (isa(op)) modulesContainingBinds.insert(moduleOp); } }; + /// Collect any port marked as being referenced via symbol. + auto collectPorts = [&](auto moduleOp) { + for (size_t p = 0, e = moduleOp.getNumArguments(); p != e; ++p) + for (auto argAttr : moduleOp.getArgAttrs(p)) + if (auto sym = argAttr.second.template dyn_cast()) + symbolCache.addDefinition(moduleOp.getNameAttr(), sym.getValue(), + moduleOp, p); + }; SmallString<32> outputPath; for (auto &op : *designOp.getBody()) { @@ -3942,6 +3980,7 @@ void SharedEmitterState::gatherFiles(bool separateModules) { .Case([&](auto mod) { // Build the IR cache. symbolCache.addDefinition(mod.getNameAttr(), mod); + collectPorts(mod); collectInstanceSymbolsAndBinds(mod); // Emit into a separate file named after the module. @@ -3966,9 +4005,10 @@ void SharedEmitterState::gatherFiles(bool separateModules) { else rootFile.ops.push_back(info); }) - .Case([&](auto op) { + .Case([&](HWModuleExternOp op) { // Build the IR cache. symbolCache.addDefinition(op.getNameAttr(), op); + collectPorts(op); if (separateModules) separateFile(op, "extern_modules.sv"); else @@ -3982,7 +4022,7 @@ void SharedEmitterState::gatherFiles(bool separateModules) { } else separateFile(op, ""); }) - .Case([&](auto schemaOp) { + .Case([&](HWGeneratorSchemaOp schemaOp) { symbolCache.addDefinition(schemaOp.getNameAttr(), schemaOp); }) .Case([&](TypeScopeOp op) { diff --git a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp index bfe3311cc39a..bb48329434de 100644 --- a/lib/Conversion/FIRRTLToHW/LowerToHW.cpp +++ b/lib/Conversion/FIRRTLToHW/LowerToHW.cpp @@ -2256,9 +2256,7 @@ LogicalResult FIRRTLLowering::visitDecl(InstanceOp oldInstance) { StringAttr symbol; if (oldInstance->getAttrOfType("lowerToBind").getValue()) { symbol = builder.getStringAttr("__" + oldInstance.name() + "__"); - auto instanceSymbol = SymbolRefAttr::get(symbol); - auto moduleSymbol = SymbolRefAttr::get(theModule.getNameAttr()); - auto bindOp = builder.create(instanceSymbol, moduleSymbol); + auto bindOp = builder.create(theModule.getNameAttr(), symbol); // If the lowered op already had output file information, then use that. // Otherwise, generate some default bind information. if (auto outputFile = oldInstance->getAttr("output_file")) diff --git a/lib/Dialect/ESI/ESIPasses.cpp b/lib/Dialect/ESI/ESIPasses.cpp index f537dd1137b1..0c29ae6dc815 100644 --- a/lib/Dialect/ESI/ESIPasses.cpp +++ b/lib/Dialect/ESI/ESIPasses.cpp @@ -583,7 +583,7 @@ void ESIPortsPass::updateInstance(HWModuleOp mod, InstanceOp inst) { // Clone the instance. b.setInsertionPointAfter(inst); auto newInst = b.create(mod, inst.instanceNameAttr(), newOperands, - inst.parameters(), inst.sym_nameAttr()); + inst.parameters(), inst.inner_symAttr()); // ----- // Wrap the results back into ESI channels and connect up all the ready @@ -819,7 +819,7 @@ void ESIPortsPass::updateInstance(HWModuleExternOp mod, InstanceOp inst) { // Create the new instance! InstanceOp newInst = instBuilder.create(mod, inst.instanceNameAttr(), newOperands, - inst.parameters(), inst.sym_nameAttr()); + inst.parameters(), inst.inner_symAttr()); // Go through the old list of non-ESI result values, and replace them with the // new non-ESI results. diff --git a/lib/Dialect/HW/HWAttributes.cpp b/lib/Dialect/HW/HWAttributes.cpp index 6bdaf7450eea..ce8447eb9cfd 100644 --- a/lib/Dialect/HW/HWAttributes.cpp +++ b/lib/Dialect/HW/HWAttributes.cpp @@ -181,6 +181,30 @@ FileListAttr FileListAttr::getFromFilename(MLIRContext *context, return FileListAttr::get(StringAttr::get(context, canonicalized)); } +//===----------------------------------------------------------------------===// +// InnerRefAttr +//===----------------------------------------------------------------------===// + +Attribute InnerRefAttr::parse(DialectAsmParser &p, Type type) { + SymbolRefAttr attr; + if (p.parseLess() || p.parseAttribute(attr) || + p.parseGreater()) + return Attribute(); + if (attr.getNestedReferences().size() != 1) + return Attribute(); + auto *context = p.getContext(); + return InnerRefAttr::get(context, attr.getRootReference(), + attr.getLeafReference()); +} + +void InnerRefAttr::print(DialectAsmPrinter &p) const { + p << getMnemonic() << "<"; + p.printSymbolName(getModule().getValue()); + p << "::"; + p.printSymbolName(getName().getValue()); + p << ">"; +} + //===----------------------------------------------------------------------===// // ParamDeclAttr //===----------------------------------------------------------------------===// diff --git a/lib/Dialect/HW/HWOps.cpp b/lib/Dialect/HW/HWOps.cpp index dcb98062fbce..e0fa0ae4f80c 100644 --- a/lib/Dialect/HW/HWOps.cpp +++ b/lib/Dialect/HW/HWOps.cpp @@ -1002,8 +1002,8 @@ static ParseResult parseInstanceOp(OpAsmParser &parser, if (succeeded(parser.parseOptionalKeyword("sym"))) { // Parsing an optional symbol name doesn't fail, so no need to check the // result. - (void)parser.parseOptionalSymbolName(sym_nameAttr, "sym_name", - result.attributes); + (void)parser.parseOptionalSymbolName( + sym_nameAttr, InnerName::getInnerNameAttrName(), result.attributes); } auto parseInputPort = [&]() -> ParseResult { @@ -1070,7 +1070,7 @@ static void printInstanceOp(OpAsmPrinter &p, InstanceOp op) { p << ' '; p.printAttributeWithoutType(op.instanceNameAttr()); - if (auto attr = op.sym_nameAttr()) { + if (auto attr = op.inner_symAttr()) { p << " sym "; p.printSymbolName(attr.getValue()); } @@ -1088,9 +1088,10 @@ static void printInstanceOp(OpAsmPrinter &p, InstanceOp op) { p << res.getType(); }); p << ')'; - p.printOptionalAttrDict(op->getAttrs(), /*elidedAttrs=*/{ - "instanceName", "sym_name", "moduleName", - "argNames", "resultNames", "parameters"}); + p.printOptionalAttrDict( + op->getAttrs(), + /*elidedAttrs=*/{"instanceName", InnerName::getInnerNameAttrName(), + "moduleName", "argNames", "resultNames", "parameters"}); } /// Return the name of the specified input port or null if it cannot be diff --git a/lib/Dialect/SV/SVOps.cpp b/lib/Dialect/SV/SVOps.cpp index 2728c23caa88..1bd2b68ddb98 100644 --- a/lib/Dialect/SV/SVOps.cpp +++ b/lib/Dialect/SV/SVOps.cpp @@ -12,6 +12,7 @@ #include "circt/Dialect/SV/SVOps.h" #include "circt/Dialect/Comb/CombOps.h" +#include "circt/Dialect/HW/HWAttributes.h" #include "circt/Dialect/HW/HWOps.h" #include "circt/Dialect/HW/HWTypes.h" #include "mlir/IR/Builders.h" @@ -119,10 +120,13 @@ static void printImplicitSSAName(OpAsmPrinter &p, Operation *op, } if (namesDisagree) - p.printOptionalAttrDict(op->getAttrs(), {SymbolTable::getSymbolAttrName()}); + p.printOptionalAttrDict(op->getAttrs(), + {SymbolTable::getSymbolAttrName(), + hw::InnerName::getInnerNameAttrName()}); else p.printOptionalAttrDict(op->getAttrs(), - {"name", SymbolTable::getSymbolAttrName()}); + {"name", SymbolTable::getSymbolAttrName(), + hw::InnerName::getInnerNameAttrName()}); } //===----------------------------------------------------------------------===// @@ -203,7 +207,7 @@ void RegOp::build(OpBuilder &builder, OperationState &odsState, name = builder.getStringAttr(""); odsState.addAttribute("name", name); if (sym_name) - odsState.addAttribute(SymbolTable::getSymbolAttrName(), sym_name); + odsState.addAttribute(hw::InnerName::getInnerNameAttrName(), sym_name); odsState.addTypes(hw::InOutType::get(elementType)); } @@ -219,7 +223,7 @@ void RegOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) { // If this reg is only written to, delete the reg and all writers. LogicalResult RegOp::canonicalize(RegOp op, PatternRewriter &rewriter) { // If the reg has a symbol, then we can't delete it. - if (op.sym_nameAttr()) + if (op.inner_symAttr()) return failure(); // Check that all operations on the wire are sv.assigns. All other wire // operations will have been handled by other canonicalization. @@ -984,7 +988,7 @@ void WireOp::build(OpBuilder &builder, OperationState &odsState, if (!name) name = builder.getStringAttr(""); if (sym_name) - odsState.addAttribute(SymbolTable::getSymbolAttrName(), sym_name); + odsState.addAttribute(hw::InnerName::getInnerNameAttrName(), sym_name); odsState.addAttribute("name", name); odsState.addTypes(InOutType::get(elementType)); @@ -1002,7 +1006,7 @@ void WireOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) { // If this wire is only written to, delete the wire and all writers. LogicalResult WireOp::canonicalize(WireOp wire, PatternRewriter &rewriter) { // If the wire has a symbol, then we can't delete it. - if (wire.sym_nameAttr()) + if (wire.inner_symAttr()) return failure(); // Wires have inout type, so they'll have assigns and read_inout operations @@ -1154,12 +1158,11 @@ LogicalResult PAssignOp::canonicalize(PAssignOp op, PatternRewriter &rewriter) { /// Instances must be at the top level of the hw.module (or within a `ifdef) // and are typically at the end of it, so we scan backwards to find them. -static hw::InstanceOp findInstanceSymbolInBlock(FlatSymbolRefAttr name, - Block *body) { +static hw::InstanceOp findInstanceSymbolInBlock(StringAttr name, Block *body) { for (auto &op : llvm::reverse(body->getOperations())) { if (auto instance = dyn_cast(op)) { - if (instance.sym_name() && - instance.sym_name().getValue() == name.getValue()) + if (instance.inner_sym() && + instance.inner_sym().getValue() == name.getValue()) return instance; } @@ -1176,8 +1179,9 @@ static hw::InstanceOp findInstanceSymbolInBlock(FlatSymbolRefAttr name, hw::InstanceOp BindOp::getReferencedInstance(const hw::SymbolCache *cache) { // If we have a cache, directly look up the referenced instance. + // FIXME if (cache) - if (auto *result = cache->getDefinition(boundInstanceAttr())) + if (auto *result = cache->getDefinition(instance().getName())) return dyn_cast(result); // Otherwise, resolve the instance by looking up the hw.module... @@ -1186,12 +1190,12 @@ hw::InstanceOp BindOp::getReferencedInstance(const hw::SymbolCache *cache) { return {}; auto hwModule = dyn_cast_or_null( - topLevelModuleOp.lookupSymbol(instanceModule())); + topLevelModuleOp.lookupSymbol(instance().getModule())); if (!hwModule) return {}; // ... then look up the instance within it. - return findInstanceSymbolInBlock(boundInstanceAttr(), + return findInstanceSymbolInBlock(instance().getName(), hwModule.getBodyBlock()); } @@ -1205,6 +1209,12 @@ static LogicalResult verifyBindOp(BindOp op) { return success(); } +void BindOp::build(OpBuilder &builder, OperationState &odsState, StringAttr mod, + StringAttr name) { + auto ref = hw::InnerRefAttr::get(mod, name); + odsState.addAttribute("instance", ref); +} + //===----------------------------------------------------------------------===// // BindInterfaceOp //===----------------------------------------------------------------------===// diff --git a/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp b/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp index 400a3024f069..1f14d0044fc4 100644 --- a/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp +++ b/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp @@ -182,8 +182,8 @@ static hw::HWModuleOp createModuleForCut(hw::HWModuleOp op, b = OpBuilder::atBlockEnd( &op->getParentOfType()->getRegion(0).front()); - auto bindOp = b.create(op.getLoc(), SymbolRefAttr::get(inst), - SymbolRefAttr::get(op.getNameAttr())); + auto bindOp = + b.create(op.getLoc(), op.getNameAttr(), inst.inner_symAttr()); if (fileName) bindOp->setAttr("output_file", fileName); return newMod; diff --git a/test/Conversion/ExportVerilog/name-legalize.mlir b/test/Conversion/ExportVerilog/name-legalize.mlir index bbcaf3c7750d..01302ab8158c 100644 --- a/test/Conversion/ExportVerilog/name-legalize.mlir +++ b/test/Conversion/ExportVerilog/name-legalize.mlir @@ -144,7 +144,7 @@ hw.module @verif_renames(%cond: i1) { } // CHECK-LABEL: module verbatim_renames( -hw.module @verbatim_renames(%a: i1) { +hw.module @verbatim_renames(%a: i1 {hw.exportPort = @asym }) { // CHECK: wire wire_0; // CHECK: // VERB Module : reg_1 inout_0 @@ -157,7 +157,7 @@ hw.module @verbatim_renames(%a: i1) { %0 = hw.instance "module" sym @struct @inout (inout: %a: i1) -> (output: i1) // CHECK: // VERB Instance : module_1 wire_0 - sv.verbatim "// VERB Instance : {{0}} {{1}}" {symbols = [@struct, @wire1]} + sv.verbatim "// VERB Instance : {{0}} {{1}} {{2}}" {symbols = [#hw.innerNameRef<@verbatim_renames::@struct>, #hw.innerNameRef<@verbatim_renames::@wire1>, #hw.innerNameRef<@verbatim_renames::@asym>]} } // CHECK-LABEL: interface output_2; diff --git a/test/Conversion/ExportVerilog/sv-dialect.mlir b/test/Conversion/ExportVerilog/sv-dialect.mlir index 79dcd430dbfc..07a3c447112e 100644 --- a/test/Conversion/ExportVerilog/sv-dialect.mlir +++ b/test/Conversion/ExportVerilog/sv-dialect.mlir @@ -22,7 +22,7 @@ hw.module @M1(%clock : i1, %cond : i1, %val : i8) { } else { // CHECK-NEXT: if (PRINTF_COND_ & 1'bx & 1'bz & 1'bz & cond & forceWire) %tmp = sv.verbatim.expr "PRINTF_COND_" : () -> i1 - %verb_tmp = sv.verbatim.expr "{{0}}" : () -> i1 {symbols = [@wire1] } + %verb_tmp = sv.verbatim.expr "{{0}}" : () -> i1 {symbols = [#hw.innerNameRef<@M1::@wire1>] } %tmp1 = sv.constantX : i1 %tmp2 = sv.constantZ : i1 %tmp3 = comb.and %tmp, %tmp1, %tmp2, %tmp2, %cond, %verb_tmp : i1 @@ -844,7 +844,7 @@ hw.module @DontDuplicateSideEffectingVerbatim() { // CHECK: automatic logic [41:0] _T = SIDEEFFECT; %tmp = sv.verbatim.expr.se "SIDEEFFECT" : () -> i42 // CHECK: automatic logic [41:0] _T_0 = b; - %verb_tmp = sv.verbatim.expr.se "{{0}}" : () -> i42 {symbols = [@regSym]} + %verb_tmp = sv.verbatim.expr.se "{{0}}" : () -> i42 {symbols = [#hw.innerNameRef<@DontDuplicateSideEffectingVerbatim::@regSym>]} // CHECK: a = _T; sv.bpassign %a, %tmp : i42 // CHECK: a = _T; @@ -877,10 +877,10 @@ hw.module @verbatim_M1(%clock : i1, %cond : i1, %val : i8) { // CHECK: MACRO(val + 8'h2A, val ^ 8'h2A reg=reg1, verbatim_M2, verbatim_inout_2, aa1,reg2 = reg2 ) sv.verbatim "MACRO({{0}}, {{1}} reg={{2}}, {{3}}, {{4}}, {{5}},reg2 = {{6}} )" (%add, %xor) : i8,i8 - {symbols = [@verbatim_reg1, @verbatim_M2, - @verbatim_inout_2, @verbatim_b1, @verbatim_reg2]} + {symbols = [#hw.innerNameRef<@verbatim_M1::@verbatim_reg1>, @verbatim_M2, + @verbatim_inout_2, #hw.innerNameRef<@verbatim_M1::@verbatim_b1>, #hw.innerNameRef<@verbatim_M1::@verbatim_reg2>]} // CHECK: Wire : wire25 - sv.verbatim " Wire : {{0}}" {symbols = [@verbatim_wireSym1]} + sv.verbatim " Wire : {{0}}" {symbols = [#hw.innerNameRef<@verbatim_M1::@verbatim_wireSym1>]} } // CHECK-LABEL: module verbatim_M2( @@ -892,7 +892,7 @@ hw.module @verbatim_M2(%clock : i1, %cond : i1, %val : i8) { // CHECK: MACRO(val + 8'h2A, val ^ 8'h2A, verbatim_M1 -- verbatim_M2) sv.verbatim "MACRO({{0}}, {{1}}, {{2}} -- {{3}})" (%add, %xor) : i8,i8 - {symbols = [@verbatim_M1, @verbatim_M2, @verbatim_b1]} + {symbols = [@verbatim_M1, @verbatim_M2, #hw.innerNameRef<@verbatim_M1::@verbatim_b1>]} } // CHECK-LABEL: module InlineAutomaticLogicInit( @@ -1031,7 +1031,7 @@ hw.module @remoteInstDut(%i: i1, %j: i1, %z: i0) -> () { } hw.module @bindInMod() { - sv.bind @bindInst in @remoteInstDut + sv.bind #hw.innerNameRef<@remoteInstDut::@bindInst> } // CHECK-LABEL: module bindInMod(); @@ -1044,7 +1044,7 @@ hw.module @bindInMod() { // CHECK-NEXT: ); // CHECK: endmodule -sv.bind @bindInst2 in @remoteInstDut +sv.bind #hw.innerNameRef<@remoteInstDut::@bindInst2> // CHECK-LABEL: bind remoteInstDut extInst a2 ( // CHECK-NEXT: ._h (mywire), diff --git a/test/Conversion/ExportVerilog/verilog-basic.mlir b/test/Conversion/ExportVerilog/verilog-basic.mlir index cb136c6cca45..0322915520fe 100644 --- a/test/Conversion/ExportVerilog/verilog-basic.mlir +++ b/test/Conversion/ExportVerilog/verilog-basic.mlir @@ -408,7 +408,7 @@ hw.module @UnaryParensIssue755(%a: i8) -> (b: i1) { hw.output %1 : i1 } -sv.bind @__BindEmissionInstance__ in @BindEmission {output_file = #hw.output_file<"BindTest/BindEmissionInstance.sv", excludeFromFileList>} +sv.bind #hw.innerNameRef<@BindEmission::@__BindEmissionInstance__> {output_file = #hw.output_file<"BindTest/BindEmissionInstance.sv", excludeFromFileList>} // CHECK-LABL: module BindEmissionInstance() hw.module @BindEmissionInstance() { hw.output diff --git a/test/Conversion/FIRRTLToHW/lower-to-hw.mlir b/test/Conversion/FIRRTLToHW/lower-to-hw.mlir index 51399c3eb939..dddbe3cec4fc 100644 --- a/test/Conversion/FIRRTLToHW/lower-to-hw.mlir +++ b/test/Conversion/FIRRTLToHW/lower-to-hw.mlir @@ -581,9 +581,9 @@ firrtl.circuit "Simple" attributes {annotations = [{class = %1455 = builtin.unrealized_conversion_cast %hits_1_7 : !firrtl.uint<1> to !firrtl.uint<1> } - // CHECK: sv.bind @[[bazSymbol:.+]] in @bindTest + // CHECK: sv.bind #hw.innerNameRef<@bindTest::@[[bazSymbol:.+]]> // CHECK-NOT: output_file - // CHECK-NEXT: sv.bind @[[quxSymbol:.+]] in @bindTest { + // CHECK-NEXT: sv.bind #hw.innerNameRef<@bindTest::@[[quxSymbol:.+]]> { // CHECK-SAME: output_file = #hw.output_file<"outputDir/bindings.sv", excludeFromFileList> // CHECK-NEXT: hw.module @bindTest() firrtl.module @bindTest() { diff --git a/test/Dialect/SV/basic.mlir b/test/Dialect/SV/basic.mlir index dfefbd9e0603..fa28bd84b1f6 100644 --- a/test/Dialect/SV/basic.mlir +++ b/test/Dialect/SV/basic.mlir @@ -219,10 +219,10 @@ hw.module @test1(%arg0: i1, %arg1: i1, %arg8: i8) { hw.output } -//CHECK-LABEL: sv.bind @a1 in @AB -//CHECK-NEXT: sv.bind @b1 in @AB -sv.bind @a1 in @AB -sv.bind @b1 in @AB +//CHECK-LABEL: sv.bind #hw.innerNameRef<@AB::@a1> +//CHECK-NEXT: sv.bind #hw.innerNameRef<@AB::@b1> +sv.bind #hw.innerNameRef<@AB::@a1> +sv.bind #hw.innerNameRef<@AB::@b1> hw.module.extern @ExternDestMod(%a: i1, %b: i2) diff --git a/test/Dialect/SV/errors.mlir b/test/Dialect/SV/errors.mlir index 5904225981e2..2ec7b8ff46f3 100644 --- a/test/Dialect/SV/errors.mlir +++ b/test/Dialect/SV/errors.mlir @@ -158,11 +158,11 @@ hw.module @Cover(%arg0: i1) { // ----- // expected-error @+1 {{Referenced instance doesn't exist}} -sv.bind @A in @Assume +sv.bind #hw.innerNameRef<@assume::@A> // ----- // expected-error @+1 {{Referenced instance doesn't exist}} -sv.bind @A in @NotAModule +sv.bind #hw.innerNameRef<@NotAModule::@A> // ----- @@ -172,7 +172,7 @@ hw.module @InternSrcMod() { hw.output } // expected-error @+1 {{Referenced instance isn't marked as doNotPrint}} -sv.bind @A in @InternSrcMod +sv.bind #hw.innerNameRef<@InternSrcMod::@A> // ----- diff --git a/test/Dialect/SV/hw-extract-test-code.mlir b/test/Dialect/SV/hw-extract-test-code.mlir index 7e03abffc306..53cb330f2324 100644 --- a/test/Dialect/SV/hw-extract-test-code.mlir +++ b/test/Dialect/SV/hw-extract-test-code.mlir @@ -24,9 +24,9 @@ // CHECK-NOT: foo_assert // CHECK-NOT: foo_assume // CHECK-NOT: foo_cover -// CHECK: sv.bind @__ETC_issue1246_assert in @issue1246 -// CHECK: sv.bind @__ETC_issue1246_assume in @issue1246 {output_file = #hw.output_file<"file4", excludeFromFileList>} -// CHECK: sv.bind @__ETC_issue1246_cover in @issue1246 +// CHECK: sv.bind #hw.innerNameRef<@issue1246::@__ETC_issue1246_assert> +// CHECK: sv.bind #hw.innerNameRef<@issue1246::@__ETC_issue1246_assume> {output_file = #hw.output_file<"file4", excludeFromFileList>} +// CHECK: sv.bind #hw.innerNameRef<@issue1246::@__ETC_issue1246_cover> module attributes {firrtl.extract.assert = #hw.output_file<"dir3/", excludeFromFileList, includeReplicatedOps>, firrtl.extract.assume.bindfile = #hw.output_file<"file4", excludeFromFileList>} { hw.module.extern @foo_cover(%a : i1) attributes {"firrtl.extract.cover.extra"} hw.module.extern @foo_assume(%a : i1) attributes {"firrtl.extract.assume.extra"}