Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[LowerClasses] Ensure classes are instantiated by an object. #7072

Merged
merged 3 commits into from
May 23, 2024
Merged

[LowerClasses] Ensure classes are instantiated by an object. #7072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c65669
Handle non-property instance objects
prithayan May 21, 2024
7fb88b3
update lit test
prithayan May 21, 2024
e68f7e0
Lookup module using name instead of op, to avoid graph lookup
prithayan May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ struct LowerClassesPass : public LowerClassesBase<LowerClassesPass> {
SymbolTable &symbolTable);

// Predicate to check if a module-like needs a Class to be created.
bool shouldCreateClass(FModuleLike moduleLike);
bool shouldCreateClass(StringAttr modName);

// Create an OM Class op from a FIRRTL Class op.
om::ClassLike createClass(FModuleLike moduleLike,
Expand All @@ -222,7 +222,7 @@ struct LowerClassesPass : public LowerClassesBase<LowerClassesPass> {
const DenseMap<StringAttr, firrtl::ClassType> &classTypeTable);

// State to memoize repeated calls to shouldCreateClass.
DenseMap<FModuleLike, bool> shouldCreateClassMemo;
DenseMap<StringAttr, bool> shouldCreateClassMemo;
};

struct PathTracker {
Expand Down Expand Up @@ -617,7 +617,8 @@ LogicalResult LowerClassesPass::processPaths(
}

// If we aren't creating a class for this child, skip this hierarchy.
if (!shouldCreateClass(it->getModule<FModuleLike>())) {
if (!shouldCreateClass(
it->getModule<FModuleLike>().getModuleNameAttr())) {
it = it.skipChildren();
continue;
}
Expand Down Expand Up @@ -656,12 +657,12 @@ void LowerClassesPass::runOnOperation() {
// Fill `shouldCreateClassMemo`.
for (auto *node : instanceGraph)
if (auto moduleLike = node->getModule<firrtl::FModuleLike>())
shouldCreateClassMemo.insert({moduleLike, false});
shouldCreateClassMemo.insert({moduleLike.getModuleNameAttr(), false});

parallelForEach(circuit.getContext(), instanceGraph,
[&](igraph::InstanceGraphNode *node) {
if (auto moduleLike = node->getModule<FModuleLike>())
shouldCreateClassMemo[moduleLike] =
shouldCreateClassMemo[moduleLike.getModuleNameAttr()] =
shouldCreateClassImpl(node);
});

Expand All @@ -682,7 +683,7 @@ void LowerClassesPass::runOnOperation() {
if (!moduleLike)
continue;

if (shouldCreateClass(moduleLike)) {
if (shouldCreateClass(moduleLike.getModuleNameAttr())) {
auto omClass = createClass(moduleLike, pathInfoTable);
auto &classLoweringState = loweringState.classLoweringStateTable[omClass];
classLoweringState.moduleLike = moduleLike;
Expand All @@ -698,7 +699,7 @@ void LowerClassesPass::runOnOperation() {
continue;
// Get the referenced module.
auto module = instance->getTarget()->getModule<FModuleLike>();
if (module && shouldCreateClass(module)) {
if (module && shouldCreateClass(module.getModuleNameAttr())) {
auto targetSym = getInnerRefTo(
{inst, 0}, [&](FModuleLike module) -> hw::InnerSymbolNamespace & {
return namespaces[module];
Expand Down Expand Up @@ -765,9 +766,9 @@ std::unique_ptr<mlir::Pass> circt::firrtl::createLowerClassesPass() {
}

// Predicate to check if a module-like needs a Class to be created.
bool LowerClassesPass::shouldCreateClass(FModuleLike moduleLike) {
bool LowerClassesPass::shouldCreateClass(StringAttr modName) {
// Return a memoized result.
return shouldCreateClassMemo.at(moduleLike);
return shouldCreateClassMemo.at(modName);
}

// Create an OM Class op from a FIRRTL Class op or Module op with properties.
Expand Down Expand Up @@ -882,13 +883,16 @@ void LowerClassesPass::lowerClass(om::ClassOp classOp, FModuleLike moduleLike,
auto propertyOperands = llvm::any_of(op.getOperandTypes(), [](Type type) {
return isa<PropertyType>(type);
});
bool needsClone = false;
if (auto instance = dyn_cast<InstanceOp>(op))
needsClone = shouldCreateClass(instance.getReferencedModuleNameAttr());
Comment on lines +886 to +888
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can lookup FModuleLike through InstanceGraph and probably we don't have to change the type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can avoid avoid an unnecessary instance graph lookup, as the module op is not required here. Querying just using the name seems better here.


// Check if any result is a property.
auto propertyResults = llvm::any_of(
op.getResultTypes(), [](Type type) { return isa<PropertyType>(type); });

// If there are no properties here, move along.
if (!propertyOperands && !propertyResults)
if (!needsClone && !propertyOperands && !propertyResults)
continue;

// Actually clone the op over to the OM Class.
Expand Down
4 changes: 4 additions & 0 deletions test/Dialect/FIRRTL/lower-classes.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@ firrtl.circuit "PathModule" {
// CHECK: %non_local = firrtl.wire sym [[NONLOCAL_SYM]] : !firrtl.uint<8>
%non_local = firrtl.wire {annotations = [{circt.nonlocal = @NonLocal, class = "circt.tracker", id = distinct[3]<>}]} : !firrtl.uint<8>
}
// CHECK: om.class @PathModule_Class(%basepath: !om.basepath) {
// CHECK: om.basepath_create %basepath
// CHECK: om.object @Child_Class
// CHECK: om.object @PathTest
// CHECK: om.class @PathTest(%basepath: !om.basepath)
firrtl.class @PathTest() {

Expand Down
Loading