diff --git a/include/circt/Dialect/OM/OMOps.td b/include/circt/Dialect/OM/OMOps.td index 6b543c8735ac..0cac1a17f4b4 100644 --- a/include/circt/Dialect/OM/OMOps.td +++ b/include/circt/Dialect/OM/OMOps.td @@ -149,8 +149,7 @@ def ObjectOp : OMOp<"object", }]; } -def ObjectFieldOp : OMOp<"object.field", - [DeclareOpInterfaceMethods, Pure]> { +def ObjectFieldOp : OMOp<"object.field", [Pure]> { let arguments = (ins ClassType:$object, FlatSymbolRefArrayAttr:$fieldPath diff --git a/include/circt/Dialect/OM/OMPasses.h b/include/circt/Dialect/OM/OMPasses.h index a29ea5ec86ad..67bbaa4b7291 100644 --- a/include/circt/Dialect/OM/OMPasses.h +++ b/include/circt/Dialect/OM/OMPasses.h @@ -21,6 +21,7 @@ namespace om { std::unique_ptr createOMLinkModulesPass(); std::unique_ptr createFreezePathsPass(); +std::unique_ptr createVerifyObjectFieldsPass(); #define GEN_PASS_REGISTRATION #include "circt/Dialect/OM/OMPasses.h.inc" diff --git a/include/circt/Dialect/OM/OMPasses.td b/include/circt/Dialect/OM/OMPasses.td index 79de81ec367d..667ff46076e2 100644 --- a/include/circt/Dialect/OM/OMPasses.td +++ b/include/circt/Dialect/OM/OMPasses.td @@ -25,3 +25,11 @@ def LinkModules: Pass<"om-link-modules", "mlir::ModuleOp"> { }]; let constructor = "circt::om::createOMLinkModulesPass()"; } + +def VerifyObjectFields: Pass<"om-verify-object-fields"> { + let summary = "Verify fields of ObjectOp are valid"; + let description = [{ + Verify object fields are valid. + }]; + let constructor = "circt::om::createVerifyObjectFieldsPass()"; +} diff --git a/lib/Dialect/OM/OMOps.cpp b/lib/Dialect/OM/OMOps.cpp index 783219ff66b4..7c1861906daa 100644 --- a/lib/Dialect/OM/OMOps.cpp +++ b/lib/Dialect/OM/OMOps.cpp @@ -330,69 +330,6 @@ circt::om::ObjectOp::verifySymbolUses(SymbolTableCollection &symbolTable) { return success(); } -//===----------------------------------------------------------------------===// -// ObjectFieldOp -//===----------------------------------------------------------------------===// - -LogicalResult -circt::om::ObjectFieldOp::verifySymbolUses(SymbolTableCollection &symbolTable) { - // Get the ObjectInstOp and the ClassLike it is an instance of. - ObjectOp objectInst = getObject().getDefiningOp(); - ClassLike classDef = cast(symbolTable.lookupNearestSymbolFrom( - *this, objectInst.getClassNameAttr())); - - // Traverse the field path, verifying each field exists. - ClassFieldLike finalField; - auto fields = SmallVector( - getFieldPath().getAsRange()); - for (size_t i = 0, e = fields.size(); i < e; ++i) { - // Verify the field exists on the ClassOp. - auto field = fields[i]; - ClassFieldLike fieldDef; - classDef.walk([&](ClassFieldLike fieldLike) { - if (fieldLike.getNameAttr() == field.getAttr()) { - fieldDef = fieldLike; - return WalkResult::interrupt(); - } - return WalkResult::advance(); - }); - if (!fieldDef) { - auto error = emitOpError("referenced non-existant field ") << field; - error.attachNote(classDef.getLoc()) << "class defined here"; - return error; - } - - // If there are more fields, verify the current field is of ClassType, and - // look up the ClassOp for that field. - if (i < e - 1) { - auto classType = dyn_cast(fieldDef.getType()); - if (!classType) - return emitOpError("nested field access into ") - << field << " requires a ClassType, but found " - << fieldDef.getType(); - - // The nested ClassOp must exist, since a field with ClassType must be - // an ObjectInstOp, which already verifies the class exists. - classDef = cast( - symbolTable.lookupNearestSymbolFrom(*this, classType.getClassName())); - - // Proceed to the next field in the path. - continue; - } - - // On the last iteration down the path, save the final field being accessed. - finalField = fieldDef; - } - - // Verify the accessed field type matches the result type. - if (finalField.getType() != getResult().getType()) - return emitOpError("expected type ") - << getResult().getType() << ", but accessed field has type " - << finalField.getType(); - - return success(); -} - //===----------------------------------------------------------------------===// // ConstantOp //===----------------------------------------------------------------------===// diff --git a/lib/Dialect/OM/Transforms/CMakeLists.txt b/lib/Dialect/OM/Transforms/CMakeLists.txt index 175e06cf17f6..b812f5536ccd 100644 --- a/lib/Dialect/OM/Transforms/CMakeLists.txt +++ b/lib/Dialect/OM/Transforms/CMakeLists.txt @@ -1,6 +1,7 @@ add_circt_dialect_library(CIRCTOMTransforms FreezePaths.cpp LinkModules.cpp + VerifyObjectFields.cpp DEPENDS CIRCTOMTransformsIncGen diff --git a/lib/Dialect/OM/Transforms/VerifyObjectFields.cpp b/lib/Dialect/OM/Transforms/VerifyObjectFields.cpp new file mode 100644 index 000000000000..f175e2af7b4d --- /dev/null +++ b/lib/Dialect/OM/Transforms/VerifyObjectFields.cpp @@ -0,0 +1,161 @@ +//===- VerifyObjectFields.cpp - Verify Object fields -------------*- C++-*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// +// +// Contains the definitions of the OM verify object fields pass. +// +//===----------------------------------------------------------------------===// + +#include "PassDetails.h" +#include "circt/Dialect/HW/HWInstanceGraph.h" +#include "circt/Dialect/HW/HWOps.h" +#include "circt/Dialect/HW/InnerSymbolTable.h" +#include "circt/Dialect/OM/OMAttributes.h" +#include "circt/Dialect/OM/OMOps.h" +#include "circt/Dialect/OM/OMPasses.h" +#include "mlir/IR/Threading.h" +#include "llvm/ADT/DenseMap.h" + +using namespace mlir; +using namespace circt; +using namespace om; + +namespace { +struct VerifyObjectFieldsPass + : public VerifyObjectFieldsBase { + void runOnOperation() override; + bool canScheduleOn(RegisteredOperationName opName) const override { + return opName.getStringRef() == "firrtl.circuit" || + opName.getStringRef() == "builtin.module"; + } +}; +} // namespace + +void VerifyObjectFieldsPass::runOnOperation() { + auto *module = getOperation(); + assert(module->getNumRegions() == 1 && + module->hasTrait() && + "op must have a single region and symbol table trait"); + auto &symbolTable = getAnalysis(); + + /// A map from a class and field name to a field. + llvm::MapVector> tables; + for (auto op : module->getRegion(0).getOps()) + tables.insert({op, llvm::DenseMap()}); + + // Peel tables parallelly. + if (failed( + mlir::failableParallelForEach(&getContext(), tables, [](auto &entry) { + ClassLike classLike = entry.first; + auto &table = entry.second; + auto result = classLike.walk([&](ClassFieldLike fieldLike) + -> WalkResult { + if (table.insert({fieldLike.getNameAttr(), fieldLike}).second) + return WalkResult::advance(); + + auto emit = fieldLike.emitOpError() + << "field " << fieldLike.getNameAttr() + << " is defined twice"; + emit.attachNote(table.lookup(fieldLike.getNameAttr()).getLoc()) + << "previous definition is here"; + return WalkResult::interrupt(); + }); + return LogicalResult::failure(result.wasInterrupted()); + }))) + return signalPassFailure(); + + // Run actual verification. Make sure not to mutate `tables`. + auto result = mlir::failableParallelForEach( + &getContext(), tables, [&tables, &symbolTable](const auto &entry) { + ClassLike classLike = entry.first; + auto result = + classLike.walk([&](ObjectFieldOp objectField) -> WalkResult { + auto objectInstType = + cast(objectField.getObject().getType()); + ClassLike classDef = + symbolTable.lookupNearestSymbolFrom( + objectField, objectInstType.getClassName()); + if (!classDef) { + objectField.emitError() + << "class " << objectInstType.getClassName() + << " was not found"; + return WalkResult::interrupt(); + } + + // Traverse the field path, verifying each field exists. + ClassFieldLike finalField; + auto fields = SmallVector( + objectField.getFieldPath().getAsRange()); + for (size_t i = 0, e = fields.size(); i < e; ++i) { + // Verify the field exists on the ClassOp. + auto field = fields[i]; + ClassFieldLike fieldDef; + auto *it = tables.find(classDef); + assert(it != tables.end() && "must be visited"); + fieldDef = it->second.lookup(field.getAttr()); + + if (!fieldDef) { + auto error = + objectField.emitOpError("referenced non-existent field ") + << field; + error.attachNote(classDef.getLoc()) << "class defined here"; + return WalkResult::interrupt(); + } + + // If there are more fields, verify the current field is of + // ClassType, and look up the ClassOp for that field. + if (i < e - 1) { + auto classType = dyn_cast(fieldDef.getType()); + if (!classType) { + objectField.emitOpError("nested field access into ") + << field << " requires a ClassType, but found " + << fieldDef.getType(); + return WalkResult::interrupt(); + } + + // Check if the nested ClassOp exists. ObjectInstOp verifier + // already checked the class exits but it's not verified yet + // if the object is an input argument. + classDef = symbolTable.lookupNearestSymbolFrom( + objectField, classType.getClassName()); + + if (!classDef) { + objectField.emitError() + << "class " << classType.getClassName() + << " was not found"; + return WalkResult::interrupt(); + } + + // Proceed to the next field in the path. + continue; + } + + // On the last iteration down the path, save the final field + // being accessed. + finalField = fieldDef; + } + + // Verify the accessed field type matches the result type. + if (finalField.getType() != objectField.getResult().getType()) { + objectField.emitOpError("expected type ") + << objectField.getResult().getType() + << ", but accessed field has type " << finalField.getType(); + + return WalkResult::interrupt(); + } + return WalkResult::advance(); + }); + return LogicalResult::failure(result.wasInterrupted()); + }); + if (failed(result)) + return signalPassFailure(); + return markAllAnalysesPreserved(); +} + +std::unique_ptr circt::om::createVerifyObjectFieldsPass() { + return std::make_unique(); +} diff --git a/lib/Firtool/Firtool.cpp b/lib/Firtool/Firtool.cpp index 3803fff1f972..a5e22ee723e6 100644 --- a/lib/Firtool/Firtool.cpp +++ b/lib/Firtool/Firtool.cpp @@ -244,6 +244,7 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm, pm.nest().addPass(firrtl::createLowerXMRPass()); pm.nest().addPass(firrtl::createLowerClassesPass()); + pm.nest().addPass(om::createVerifyObjectFieldsPass()); // Check for static asserts. pm.nest().nest().addPass( @@ -261,6 +262,9 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm, // Check inner symbols and inner refs. pm.addPass(hw::createVerifyInnerRefNamespacePass()); + // Check OM object fields. + pm.addPass(om::createVerifyObjectFieldsPass()); + return success(); } @@ -308,6 +312,9 @@ LogicalResult firtool::populateHWToSV(mlir::PassManager &pm, // Check inner symbols and inner refs. pm.addPass(hw::createVerifyInnerRefNamespacePass()); + // Check OM object fields. + pm.addPass(om::createVerifyObjectFieldsPass()); + return success(); } @@ -341,6 +348,9 @@ populatePrepareForExportVerilog(mlir::PassManager &pm, // Check inner symbols and inner refs. pm.addPass(hw::createVerifyInnerRefNamespacePass()); + // Check OM object fields. + pm.addPass(om::createVerifyObjectFieldsPass()); + return success(); } } // namespace detail diff --git a/test/Dialect/OM/errors.mlir b/test/Dialect/OM/errors.mlir index a1eea10de9cc..ac7cb79ebcd4 100644 --- a/test/Dialect/OM/errors.mlir +++ b/test/Dialect/OM/errors.mlir @@ -1,4 +1,4 @@ -// RUN: circt-opt %s -verify-diagnostics -split-input-file +// RUN: circt-opt -om-verify-object-fields %s -verify-diagnostics -split-input-file om.class @Class() { // expected-error @+1 {{'om.object' op result type ("Bar") does not match referred to class ("Foo")}} @@ -39,7 +39,7 @@ om.class @Class1() {} om.class @Class2() { %0 = om.object @Class1() : () -> !om.class.type<@Class1> - // expected-error @+1 {{'om.object.field' op referenced non-existant field @foo}} + // expected-error @+1 {{'om.object.field' op referenced non-existent field @foo}} om.object.field %0, [@foo] : (!om.class.type<@Class1>) -> i1 } @@ -124,3 +124,20 @@ om.class @BadPath(%basepath: !om.basepath) { // expected-error @below {{invalid symbol reference}} %0 = om.path_create reference %basepath @Thing } + + +// ----- + +om.class @DupField(%0: i1) { + // expected-note @+1 {{previous definition is here}} + om.class.field @foo, %0 : i1 + // expected-error @+1 {{'om.class.field' op field "foo" is defined twice}} + om.class.field @foo, %0 : i1 +} + +// ----- + +om.class @UnknownClass(%arg: !om.class.type<@Unknwon>) { + // expected-error @+1 {{class @Unknwon was not found}} + om.object.field %arg, [@unknown]: (!om.class.type<@Unknwon>) -> i1 +}