terapines
/
circt
mirror of https://github.com/llvm/circt.git
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[FIRRTL] Resolve portAnnotations/arg_attrs ambiguity (#1645) 

Most of the FIRRTL code base expects port annotations to live in the
`portAnnotations` attribute on `FModuleOp` and `FExtModuleOp`. But the
parser, printer, and some operation builders accidentally store these in
`arg_attrs` through the standard argument attribute mechanism of MLIR.

This commit removes redundant/conflicting uses of `arg_attrs` and
`portAnnotations` in the code base. To keep things ergonomic for the
custom syntax of FIRRTL operations, we still parse and print port
annotations as part of the regular argument attributes (under the
`firrtl.annotations` key), but then immediately separate them out into
the `portAnnotations` attribute where all of our code expects this to
live.
This commit is contained in:
Fabian Schuiki 2021-08-27 21:55:22 +02:00 committed by GitHub
parent 49e5cdd668
commit 8b3aa8c61b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 95 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
include/circt/Dialect/FIRRTL/FIRRTLStructure.td
View File

@ -69,8 +69,7 @@ def FModuleOp : FIRRTLOp<"module",
let skipDefaultBuilders = 1;
let builders = [
OpBuilder<(ins "StringAttr":$name, "ArrayRef<ModulePortInfo>":$ports,
CArg<"ArrayAttr","ArrayAttr()">:$annotations,
CArg<"ArrayAttr","ArrayAttr()">:$portAnnotations)>
CArg<"ArrayAttr","ArrayAttr()">:$annotations)>
];
let extraClassDeclaration = [{
@ -151,8 +150,7 @@ def FExtModuleOp : FIRRTLOp<"extmodule",
OpBuilder<(ins "StringAttr":$name,
"ArrayRef<ModulePortInfo>":$ports,
CArg<"StringRef", "StringRef()">:$defnamAttr,
CArg<"ArrayAttr", "ArrayAttr()">:$annotations,
CArg<"ArrayAttr","ArrayAttr()">:$portAnnotations)>
CArg<"ArrayAttr", "ArrayAttr()">:$annotations)>
];
let extraClassDeclaration = [{

83
lib/Dialect/FIRRTL/FIRRTLOps.cpp
View File

@ -470,7 +470,7 @@ void FModuleOp::erasePorts(ArrayRef<unsigned> portIndices) {
static void buildModule(OpBuilder &builder, OperationState &result,
StringAttr name, ArrayRef<ModulePortInfo> ports,
ArrayAttr annotations, ArrayAttr portAnnotations) {
ArrayAttr annotations) {
using namespace mlir::function_like_impl;
// Add an attribute for the name.
@ -485,17 +485,17 @@ static void buildModule(OpBuilder &builder, OperationState &result,
result.addAttribute(getTypeAttrName(), TypeAttr::get(type));
// Record the names of the arguments if present.
SmallVector<Attribute, 4> argAttrs;
SmallVector<Attribute, 4> portAnnotations;
SmallVector<Attribute, 4> portNames;
SmallVector<Direction, 4> portDirections;
for (size_t i = 0, e = ports.size(); i != e; ++i) {
portNames.push_back(ports[i].name);
portDirections.push_back(ports[i].direction);
argAttrs.push_back(ports[i].annotations.getArgumentAttrDict());
portAnnotations.push_back(ports[i].annotations.getArrayAttr());
}
// Both attributes are added, even if the module has no ports.
result.addAttribute(getArgDictAttrName(), builder.getArrayAttr(argAttrs));
result.addAttribute("portAnnotations", builder.getArrayAttr(portAnnotations));
result.addAttribute("portNames", builder.getArrayAttr(portNames));
result.addAttribute(
direction::attrKey,
@ -505,17 +505,13 @@ static void buildModule(OpBuilder &builder, OperationState &result,
annotations = builder.getArrayAttr({});
result.addAttribute("annotations", annotations);
if (!portAnnotations)
portAnnotations = builder.getArrayAttr({});
result.addAttribute("portAnnotations", portAnnotations);
result.addRegion();
}
void FModuleOp::build(OpBuilder &builder, OperationState &result,
StringAttr name, ArrayRef<ModulePortInfo> ports,
ArrayAttr annotations, ArrayAttr portAnnotations) {
buildModule(builder, result, name, ports, annotations, portAnnotations);
ArrayAttr annotations) {
buildModule(builder, result, name, ports, annotations);
// Create a region and a block for the body.
auto *bodyRegion = result.regions[0].get();
@ -529,9 +525,8 @@ void FModuleOp::build(OpBuilder &builder, OperationState &result,
void FExtModuleOp::build(OpBuilder &builder, OperationState &result,
StringAttr name, ArrayRef<ModulePortInfo> ports,
StringRef defnameAttr, ArrayAttr annotations,
ArrayAttr portAnnotations) {
buildModule(builder, result, name, ports, annotations, portAnnotations);
StringRef defnameAttr, ArrayAttr annotations) {
buildModule(builder, result, name, ports, annotations);
if (!defnameAttr.empty())
result.addAttribute("defname", builder.getStringAttr(defnameAttr));
}
@ -541,7 +536,7 @@ void FExtModuleOp::build(OpBuilder &builder, OperationState &result,
static void printFunctionSignature2(OpAsmPrinter &p, Operation *op,
ArrayRef<Type> argTypes, bool isVariadic,
ArrayRef<Type> resultTypes,
bool &needportNamesAttr, APInt directions) {
bool &needPortNamesAttr, APInt directions) {
Region &body = op->getRegion(0);
bool isExternal = body.empty();
SmallString<32> resultNameStr;
@ -564,14 +559,20 @@ static void printFunctionSignature2(OpAsmPrinter &p, Operation *op,
// If the name wasn't printable in a way that agreed with portName, make
// sure to print out an explicit portNames attribute.
if (!portName.empty() && tmpStream.str().drop_front() != portName)
needportNamesAttr = true;
needPortNamesAttr = true;
p << tmpStream.str() << ": ";
} else if (!portName.empty()) {
p << '%' << portName << ": ";
}
p.printType(argTypes[i]);
p.printOptionalAttrDict(::mlir::function_like_impl::getArgAttrs(op, i));
// Combine the port's annos in `portAnnotations` with its attributes in
// `arg_attrs` to print a uniform attribute dictionary of the form
// `{firrtl.annotations = [<annos>], <arg-attrs>}`.
auto argAttrs = ::mlir::function_like_impl::getArgAttrs(op, i);
auto annos = AnnotationSet::forPort(op, i);
p.printOptionalAttrDict(annos.getArgumentAttrDict(argAttrs).getValue());
}
if (isVariadic) {
@ -587,6 +588,7 @@ static ParseResult parseFunctionArgumentList2(
OpAsmParser &parser, bool allowAttributes, bool allowVariadic,
SmallVectorImpl<OpAsmParser::OperandType> &argNames,
SmallVectorImpl<Type> &argTypes, SmallVectorImpl<Direction> &argDirections,
SmallVectorImpl<Attribute> &argAnnotations,
SmallVectorImpl<NamedAttrList> &argAttrs, bool &isVariadic) {
if (parser.parseLParen())
return failure();
@ -633,6 +635,10 @@ static ParseResult parseFunctionArgumentList2(
return failure();
if (!allowAttributes && !attrs.empty())
return parser.emitError(loc, "expected arguments without attributes");
Attribute annos = attrs.erase(getDialectAnnotationAttrName());
if (!annos)
annos = ArrayAttr::get(parser.getBuilder().getContext(), {});
argAnnotations.push_back(annos);
argAttrs.push_back(attrs);
return success();
};
@ -693,12 +699,14 @@ parseFunctionSignature2(OpAsmParser &parser, bool allowVariadic,
SmallVectorImpl<OpAsmParser::OperandType> &argNames,
SmallVectorImpl<Type> &argTypes,
SmallVectorImpl<Direction> &argDirections,
SmallVectorImpl<Attribute> &argAnnotations,
SmallVectorImpl<NamedAttrList> &argAttrs,
bool &isVariadic, SmallVectorImpl<Type> &resultTypes,
SmallVectorImpl<NamedAttrList> &resultAttrs) {
bool allowArgAttrs = true;
if (parseFunctionArgumentList2(parser, allowArgAttrs, allowVariadic, argNames,
argTypes, argDirections, argAttrs, isVariadic))
argTypes, argDirections, argAnnotations,
argAttrs, isVariadic))
return failure();
if (succeeded(parser.parseOptionalArrow()))
return parseFunctionResultList2(parser, resultTypes, resultAttrs);
@ -722,17 +730,18 @@ static void printModuleLikeOp(OpAsmPrinter &p, Operation *op) {
p << op->getName() << ' ';
p.printSymbolName(funcName);
bool needportNamesAttr = false;
bool needPortNamesAttr = false;
printFunctionSignature2(p, op, argTypes, /*isVariadic*/ false, resultTypes,
needportNamesAttr,
needPortNamesAttr,
getModulePortDirections(op).getValue());
SmallVector<StringRef, 3> omittedAttrs({direction::attrKey});
if (!needportNamesAttr)
if (!needPortNamesAttr)
omittedAttrs.push_back("portNames");
if (op->getAttrOfType<ArrayAttr>("annotations").empty())
omittedAttrs.push_back("annotations");
if (op->getAttrOfType<ArrayAttr>("portAnnotations").empty())
omittedAttrs.push_back("portAnnotations");
// Port annotations are printed in as part of the signature already.
omittedAttrs.push_back("portAnnotations");
printFunctionAttributes(p, op, argTypes.size(), resultTypes.size(),
omittedAttrs);
@ -768,6 +777,7 @@ static ParseResult parseFModuleOp(OpAsmParser &parser, OperationState &result,
SmallVector<Type, 4> argTypes;
SmallVector<Type, 4> resultTypes;
SmallVector<Direction, 4> argDirections;
SmallVector<Attribute, 4> argAnnotations;
auto &builder = parser.getBuilder();
// Parse the name as a symbol.
@ -778,9 +788,9 @@ static ParseResult parseFModuleOp(OpAsmParser &parser, OperationState &result,
// Parse the function signature.
bool isVariadic = false;
if (parseFunctionSignature2(parser, /*allowVariadic*/ false, entryArgs,
argTypes, argDirections, portNamesAttrs,
isVariadic, resultTypes, resultAttrs))
if (parseFunctionSignature2(
parser, /*allowVariadic*/ false, entryArgs, argTypes, argDirections,
argAnnotations, portNamesAttrs, isVariadic, resultTypes, resultAttrs))
return failure();
// Record the argument and result types as an attribute. This is necessary
@ -801,6 +811,15 @@ static ParseResult parseFModuleOp(OpAsmParser &parser, OperationState &result,
result.addAttribute(direction::attrKey,
direction::packAttribute(argDirections, context));
// Add the port annotations attribute.
if (!result.attributes.get("portAnnotations")) {
auto emptyArray = ArrayAttr::get(context, {});
if (llvm::any_of(argAnnotations,
[&](auto anno) { return anno != emptyArray; }))
result.addAttribute("portAnnotations",
ArrayAttr::get(context, argAnnotations));
}
SmallVector<Attribute> portNames;
if (!result.attributes.get("portNames")) {
// Postprocess each of the arguments. If there was no portNames
@ -852,10 +871,22 @@ static ParseResult parseFExtModuleOp(OpAsmParser &parser,
}
static LogicalResult verifyModuleSignature(Operation *op) {
for (auto argType : getModuleType(op).getInputs()) {
const auto &inputs = getModuleType(op).getInputs();
for (auto argType : inputs) {
if (!argType.isa<FIRRTLType>())
return op->emitOpError("all module ports must be firrtl types");
}
// Arguments must not have a `firrtl.annotations` attribute. The module
// overall has a `portAnnotations` attribute that captures these.
for (unsigned i = 0, e = inputs.size(); i < e; ++i) {
auto dict = mlir::function_like_impl::getArgAttrDict(op, i);
if (dict && dict.get("firrtl.annotations"))
return op->emitOpError(
"port annotations must be in the module's `portAnnotations` attr, "
"not the `firrtl.annotations` arg attr");
}
return success();
}

6
lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp
View File

@ -340,12 +340,10 @@ void GrandCentralVisitor::handlePorts(Operation *op) {
for (size_t i = 0, e = ports.size(); i != e; ++i) {
auto port = ports[i];
handleRefLike(op, port.annotations, port.type);
newArgAttrs.push_back(port.annotations.applyToPortDictionaryAttr(
DictionaryAttr::get(op->getContext(), {})));
newArgAttrs.push_back(port.annotations.getArrayAttr());
}
mlir::function_like_impl::setAllArgAttrDicts(op, newArgAttrs);
op->setAttr("portAnnotations", ArrayAttr::get(op->getContext(), newArgAttrs));
}
void GrandCentralVisitor::visitDecl(InstanceOp op) {

18
lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp
View File

@ -707,19 +707,21 @@ void TypeLoweringVisitor::visitDecl(FExtModuleOp extModule) {
// Drop old "portNames", directions, and argument attributes. These are
// handled differently below.
if (attr.first != "portNames" && attr.first != direction::attrKey &&
attr.first != "portAnnotations" &&
attr.first != mlir::function_like_impl::getArgDictAttrName())
newModuleAttrs.push_back(attr);
SmallVector<Attribute> newArgNames;
SmallVector<Direction> newArgDirections;
SmallVector<Attribute, 8> newArgAttrs;
SmallVector<Attribute, 8> newArgAnnotations;
SmallVector<Type, 8> inputTypes;
for (auto &port : newArgs) {
newArgNames.push_back(port.first.name);
newArgDirections.push_back(port.first.direction);
newArgAttrs.push_back(
port.first.annotations.getArgumentAttrDict(port.second));
newArgAttrs.push_back(builder.getDictionaryAttr(port.second));
newArgAnnotations.push_back(port.first.annotations.getArrayAttr());
inputTypes.push_back(port.first.type);
}
newModuleAttrs.push_back(NamedAttribute(Identifier::get("portNames", context),
@ -727,6 +729,9 @@ void TypeLoweringVisitor::visitDecl(FExtModuleOp extModule) {
newModuleAttrs.push_back(
NamedAttribute(Identifier::get(direction::attrKey, context),
direction::packAttribute(newArgDirections, context)));
newModuleAttrs.push_back(
NamedAttribute(Identifier::get("portAnnotations", context),
builder.getArrayAttr(newArgAnnotations)));
// Attach new argument attributes.
newModuleAttrs.push_back(NamedAttribute(
@ -781,23 +786,28 @@ void TypeLoweringVisitor::visitDecl(FModuleOp module) {
// Drop old "portNames", directions, and argument attributes. These are
// handled differently below.
if (attr.first != "portNames" && attr.first != direction::attrKey &&
attr.first != "portAnnotations" &&
attr.first != mlir::function_like_impl::getArgDictAttrName())
newModuleAttrs.push_back(attr);
SmallVector<Attribute> newArgNames;
SmallVector<Direction> newArgDirections;
SmallVector<Attribute, 8> newArgAttrs;
SmallVector<Attribute, 8> newArgAnnotations;
for (auto &port : newArgs) {
newArgNames.push_back(port.first.name);
newArgDirections.push_back(port.first.direction);
newArgAttrs.push_back(
port.first.annotations.getArgumentAttrDict(port.second));
newArgAttrs.push_back(builder->getDictionaryAttr(port.second));
newArgAnnotations.push_back(port.first.annotations.getArrayAttr());
}
newModuleAttrs.push_back(NamedAttribute(Identifier::get("portNames", context),
builder->getArrayAttr(newArgNames)));
newModuleAttrs.push_back(
NamedAttribute(Identifier::get(direction::attrKey, context),
direction::packAttribute(newArgDirections, context)));
newModuleAttrs.push_back(
NamedAttribute(Identifier::get("portAnnotations", context),
builder->getArrayAttr(newArgAnnotations)));
// Attach new argument attributes.
newModuleAttrs.push_back(NamedAttribute(

20
test/Dialect/FIRRTL/annotations-generic.fir Normal file
View File

@ -0,0 +1,20 @@
; RUN: circt-translate --import-firrtl --mlir-print-op-generic --split-input-file %s | FileCheck %s
; A ReferenceTarget/ComponentName pointing at a module/extmodule port should work.
circuit Foo: %[[{"a":"a","target":"~Foo|Bar>bar"},{"b":"b","target":"Foo.Foo.foo"}]]
extmodule Bar:
input bar: UInt<1>
module Foo:
input foo: UInt<1>
inst bar of Bar
bar.bar <= foo
; CHECK-LABEL: "firrtl.extmodule"() ( {
; CHECK: }) {
; CHECK-SAME: portAnnotations = {{['[']['[']}}{a = "a"}]]
; CHECK-SAME: sym_name = "Bar"
; CHECK-LABEL: "firrtl.module"() ( {
; CHECK: }) {
; CHECK-SAME: portAnnotations = {{['[']['[']}}{b = "b"}]]
; CHECK-SAME: sym_name = "Foo"