From 85e6d6adc58812ee66b6b1e2a7fdc3f98bb219d6 Mon Sep 17 00:00:00 2001 From: Lenny Truong Date: Fri, 4 Oct 2024 11:25:48 -0700 Subject: [PATCH] Add fields location API --- include/circt/Dialect/OM/OMOps.td | 22 +++++++++++++ .../FIRRTL/Transforms/LowerClasses.cpp | 11 +++---- lib/Dialect/OM/Evaluator/Evaluator.cpp | 11 +------ lib/Dialect/OM/OMOps.cpp | 32 +++++++++++++++++++ 4 files changed, 59 insertions(+), 17 deletions(-) diff --git a/include/circt/Dialect/OM/OMOps.td b/include/circt/Dialect/OM/OMOps.td index 59496a737af0..cac3526c2da1 100644 --- a/include/circt/Dialect/OM/OMOps.td +++ b/include/circt/Dialect/OM/OMOps.td @@ -112,6 +112,28 @@ def ClassOp : OMClassLike<"class", [ circt::om::ClassFieldsOp getFieldsOp() { return mlir::cast(this->getBodyBlock()->getTerminator()); } + + // The addFields API encapsulates the logic used to represent field + // locations under the hood. Users should invoke this method rather + // than construct the operation directly, otherwise logic to retrieve + // the field location will break. + // This is required because MLIR's FusedLoc uses a "set" semantics where a + // single location is used to represent multiple fields with the same + // location. The OM implementation uses the metadata attribute on FusedLoc + // to store the original array of locations, so that the specific location + // of a field may be easily retrieved by index using the + // `getFieldLocByIndex` API. + void addFields(mlir::OpBuilder &builder, mlir::ArrayRef + locs, mlir::ArrayRef values); + + // Return the location for a field referenced by index in the fieldNames + // array attribute. If the field has a location added by addFields API, + // its location will be retrieved from the array of per field locations. + // Otherwise, it will inherit the location of the class op Using this with + // a ClassFieldsOp that has been constructed with a FusedLoc but not + // following the internal storage format of `addFields` will result in an + // assertion error + mlir::Location getFieldLocByIndex(size_t i); }]; } diff --git a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp index a53cfa71942c..08e9d6ae89ec 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp @@ -1151,7 +1151,7 @@ void LowerClassesPass::lowerClass(om::ClassOp classOp, FModuleLike moduleLike, // Clone the property ops from the FIRRTL Class or Module to the OM Class. SmallVector opsToErase; OpBuilder builder = OpBuilder::atBlockBegin(classOp.getBodyBlock()); - llvm::SmallVector fieldLocs; + llvm::SmallVector fieldLocs; llvm::SmallVector fieldValues; for (auto &op : moduleLike->getRegion(0).getOps()) { // Check if any operand is a property. @@ -1173,7 +1173,7 @@ void LowerClassesPass::lowerClass(om::ClassOp classOp, FModuleLike moduleLike, if (isa(op) && dyn_cast(cast(op).getDest())) { // Store any output property assignments into fields op inputs. - fieldLocs.push_back(LocationAttr(op.getLoc())); + fieldLocs.push_back(op.getLoc()); fieldValues.push_back(mapping.lookup(cast(op).getSrc())); } else { // Clone the op over to the OM Class. @@ -1190,14 +1190,11 @@ void LowerClassesPass::lowerClass(om::ClassOp classOp, FModuleLike moduleLike, if (hasContainingModule) { BlockArgument argumentValue = classBody->addArgument( getRtlPortsType(&getContext()), UnknownLoc::get(&getContext())); - fieldLocs.push_back(LocationAttr(argumentValue.getLoc())); + fieldLocs.push_back(argumentValue.getLoc()); fieldValues.push_back(argumentValue); } - // TODO: Store set of locs? - // TODO: Document FusedLoc logic - builder.create( - builder.getFusedLoc({}, builder.getArrayAttr(fieldLocs)), fieldValues); + classOp.addFields(builder, fieldLocs, fieldValues); // If the module-like is a Class, it will be completely erased later. // Otherwise, erase just the property ports and ops. diff --git a/lib/Dialect/OM/Evaluator/Evaluator.cpp b/lib/Dialect/OM/Evaluator/Evaluator.cpp index a5074bd58ede..9eab2d736f0c 100644 --- a/lib/Dialect/OM/Evaluator/Evaluator.cpp +++ b/lib/Dialect/OM/Evaluator/Evaluator.cpp @@ -16,7 +16,6 @@ #include "mlir/IR/SymbolTable.h" #include "llvm/ADT/TypeSwitch.h" #include "llvm/Support/Debug.h" -#include #define DEBUG_TYPE "om-evaluator" @@ -260,18 +259,10 @@ circt::om::Evaluator::evaluateObjectInstance(StringAttr className, auto fieldNames = cls.getFieldNames(); auto operands = cls.getFieldsOp()->getOperands(); - auto fieldsLoc = cls.getFieldsOp()->getLoc(); for (size_t i = 0; i < fieldNames.size(); ++i) { auto name = fieldNames[i]; auto value = operands[i]; - Location fieldLoc = fieldsLoc; - if (auto locs = dyn_cast(fieldLoc)) { - // TODO: Document FusedLoc logic - assert(dyn_cast(locs.getMetadata())); - assert(dyn_cast(cast(locs.getMetadata())[i])); - fieldLoc = - Location(cast(cast(locs.getMetadata())[i])); - } + auto fieldLoc = cls.getFieldLocByIndex(i); FailureOr result = evaluateValue(value, actualParams, fieldLoc); if (failed(result)) diff --git a/lib/Dialect/OM/OMOps.cpp b/lib/Dialect/OM/OMOps.cpp index 42ecde5113bb..b58e5a828ca8 100644 --- a/lib/Dialect/OM/OMOps.cpp +++ b/lib/Dialect/OM/OMOps.cpp @@ -333,6 +333,38 @@ void circt::om::ClassOp::replaceFieldTypes(AttrTypeReplacer replacer) { replaceClassLikeFieldTypes(*this, replacer); } +void circt::om::ClassOp::addFields(mlir::OpBuilder &builder, + mlir::ArrayRef locs, + mlir::ArrayRef values) { + // Store the original locations as a metadata array so that unique locations + // are preserved as a mapping from field index to location + mlir::SmallVector locAttrs; + for (auto loc : locs) { + locAttrs.push_back(cast(LocationAttr(loc))); + } + // Also store the locations incase there's some other analysis that might + // be able to use the default FusedLoc representation. + builder.create( + builder.getFusedLoc(locs, builder.getArrayAttr(locAttrs)), values); +} + +mlir::Location circt::om::ClassOp::getFieldLocByIndex(size_t i) { + Location loc = this->getFieldsOp()->getLoc(); + if (auto locs = dyn_cast(loc)) { + // Because it's possible for a user to construct a fields op directly and + // place a FusedLoc that doersn't follow the storage format of addFields, we + // assert the information has been stored appropriately + ArrayAttr metadataArr = dyn_cast(locs.getMetadata()); + assert(metadataArr && "Expected fused loc to store metadata array"); + assert(i < metadataArr.size() && + "expected index to be less than array size"); + LocationAttr locAttr = dyn_cast(metadataArr[i]); + assert(locAttr && "expected metadataArr entry to be location attribute"); + loc = Location(locAttr); + } + return loc; +} + //===----------------------------------------------------------------------===// // ClassExternOp //===----------------------------------------------------------------------===//