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

[FIRRTL] Allow local targets to be multiply-instantiated. #7613

Merged
merged 3 commits into from
Sep 21, 2024
Merged

[FIRRTL] Allow local targets to be multiply-instantiated. #7613

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
103090a
[FIRRTL] Allow local targets to be multiply-instantiated.
mikeurbach Sep 19, 2024
71d7792
Update lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp
mikeurbach Sep 20, 2024
4d28fb7
Update comment about ambiguity.
mikeurbach Sep 20, 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
26 changes: 17 additions & 9 deletions lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ struct PathTracker {
// and `owningModule`.
FailureOr<bool> getOrComputeNeedsAltBasePath(Location loc,
StringAttr moduleName,
FModuleOp owningModule);
FModuleOp owningModule,
bool isNonLocal);
FModuleLike module;

// Local data structures.
Expand Down Expand Up @@ -364,7 +365,8 @@ LogicalResult PathTracker::runOnModule() {

FailureOr<bool>
PathTracker::getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName,
FModuleOp owningModule) {
FModuleOp owningModule,
bool isNonLocal) {

auto it = needsAltBasePathCache.find({moduleName, owningModule});
if (it != needsAltBasePathCache.end())
Expand All @@ -383,9 +385,9 @@ PathTracker::getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName,
needsAltBasePath = true;
break;
}
// If there is more than one instance of this module, then the path
// operation is ambiguous, which is an error.
if (!node->hasOneUse()) {
// If there is more than one instance of this module, and the target is
// non-local, then the path operation is ambiguous, which is an error.
if (isNonLocal && !node->hasOneUse()) {
auto diag = mlir::emitError(loc)
<< "unable to uniquely resolve target due "
"to multiple instantiation";
Expand Down Expand Up @@ -517,8 +519,8 @@ PathTracker::processPathTrackers(const AnnoTarget &target) {
}

// Check if we need an alternative base path.
auto needsAltBasePath =
getOrComputeNeedsAltBasePath(op->getLoc(), moduleName, owningModule);
auto needsAltBasePath = getOrComputeNeedsAltBasePath(
op->getLoc(), moduleName, owningModule, hierPathOp);
if (failed(needsAltBasePath)) {
error = true;
return false;
Expand All @@ -528,6 +530,10 @@ PathTracker::processPathTrackers(const AnnoTarget &target) {
// to the start of the annotation's NLA.
InstanceGraphNode *node = instanceGraph.lookup(moduleName);
while (true) {
// If it's not a non-local target, we don't have to append anything.
if (!hierPathOp)
break;

// If we get to the owning module or the top, we're done.
if (node->getModule() == owningModule || node->noUses())
break;
Expand Down Expand Up @@ -582,8 +588,10 @@ LogicalResult PathTracker::updatePathInfoTable(PathInfoTable &pathInfoTable,
auto &pathInfo = it->second;
if (!inserted) {
assert(pathInfo.loc.has_value() && "all PathInfo should have a Location");
auto diag = emitError(pathInfo.loc.value(), "duplicate identifier found");
diag.attachNote(entry.op->getLoc()) << "other identifier here";
auto diag = emitError(pathInfo.loc.value(),
"path identifier already found, paths must resolve "
"to a unique target");
diag.attachNote(entry.op->getLoc()) << "other path identifier here";
return failure();
}

Expand Down
11 changes: 5 additions & 6 deletions lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ struct PathResolver {
const AnnoPathValue &target,
FlatSymbolRefAttr &result) {

// If the path is empty then this is a local reference and we should not
// construct a HierPathOp.
if (target.instances.empty())
return success();

// We want to root this path at the top level module, or in the case of an
// unreachable module, we settle for as high as we can get.
auto module = target.ref.getModule();
Expand Down Expand Up @@ -69,12 +74,6 @@ struct PathResolver {
node = (*node->usesBegin())->getParent();
}

// If the path is empty then this is a local reference and we should not
// construct a HierPathOp.
if (target.instances.empty()) {
return success();
}

// Transform the instances into a list of FlatSymbolRefs.
SmallVector<Attribute> insts;
insts.reserve(target.instances.size());
Expand Down
4 changes: 2 additions & 2 deletions test/Dialect/FIRRTL/lower-classes-errors.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ firrtl.circuit "PathIllegalHierpath" {

firrtl.circuit "PathDuplicateID" {
firrtl.module @PathDuplicateID() {
// expected-error @below {{duplicate identifier found}}
// expected-error @below {{path identifier already found, paths must resolve to a unique target}}
%a = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} : !firrtl.uint<8>
// expected-note @below {{other identifier here}}
// expected-note @below {{other path identifier here}}
%b = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} : !firrtl.uint<8>
}
}
Expand Down
25 changes: 24 additions & 1 deletion test/Dialect/FIRRTL/lower-classes.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ firrtl.circuit "PathModule" {
// CHECK: hw.hierpath private [[WIRE_PATH:@.+]] [@PathModule::[[WIRE_SYM:@.+]]]
// CHECK: hw.hierpath private [[VECTOR_PATH:@.+]] [@PathModule::[[VECTOR_SYM:@.+]]]
// CHECK: hw.hierpath private [[INST_PATH:@.+]] [@PathModule::@child]
// CHECK: hw.hierpath private [[LOCAL_PATH:@.+]] [@Child]
// CHECK: hw.hierpath private [[MODULE_PATH:@.+]] [@PathModule::@child, @Child::[[NONLOCAL_SYM:@.+]]]

// CHECK: firrtl.module @PathModule(in %in: !firrtl.uint<1> sym [[PORT_SYM]]) {
Expand Down Expand Up @@ -209,7 +210,7 @@ firrtl.circuit "PathModule" {
// CHECK: om.path_create member_instance %basepath [[INST_PATH]]
// CHECK: om.path_create member_instance %basepath [[INST_PATH]]
// CHECK: om.path_create instance %basepath [[INST_PATH]]
// CHECK: om.path_create reference %basepath [[NONLOCAL_PATH]]
// CHECK: om.path_create reference %basepath [[LOCAL_PATH]]
%instance_member_instance = firrtl.path member_instance distinct[4]<>
%instance_member_reference = firrtl.path member_reference distinct[4]<>
%instance = firrtl.path instance distinct[4]<>
Expand Down Expand Up @@ -471,3 +472,25 @@ firrtl.circuit "PathTargetReplaced" {
firrtl.module private @WillBeReplaced(out %output: !firrtl.integer) {
}
}

// CHECK-LABEL: firrtl.circuit "LocalPath"
firrtl.circuit "LocalPath" {
// CHECK: hw.hierpath private [[MODULE_NLA:@.+]] [@Child]
// CHECK: hw.hierpath private [[WIRE_NLA:@.+]] [@Child::[[WIRE_SYM:@.+]]]

firrtl.module @Child() attributes {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} {
// CHECK: firrtl.wire sym [[WIRE_SYM]]
%wire = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[1]<>}]} : !firrtl.uint<8>
}

firrtl.module @LocalPath() {
// CHECK: om.path_create instance %basepath [[MODULE_NLA]]
%0 = firrtl.path instance distinct[0]<>

// CHECK: om.path_create reference %basepath [[WIRE_NLA]]
%1 = firrtl.path reference distinct[1]<>

firrtl.instance child0 @Child()
firrtl.instance child1 @Child()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

To elaborate on my comment... What I'm concerned about is if there is a way to rewrite this circuit such that @Child is duplicated into @Child0 and @Child1. What are the trackers used then? After deduplication, it would seem like this would result in non-local annotation trackers which would then trip the error checking logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. The main way I have noticed something like this happening is when PrefixModules clones modules. You can set up an example that would cause @Child to @Child0 and @Child1, each with a local annotation with the same distinct ID. This would then error out in LowerClasses here:

circt/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp

Lines 585 to 586 in afd61f2

auto diag = emitError(pathInfo.loc.value(), "duplicate identifier found");
diag.attachNote(entry.op->getLoc()) << "other identifier here";

Which is equivalent to this existing check:

circt/lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

Lines 650 to 654 in afd61f2

auto diag = op->emitError(omirTrackerAnnoClass)
<< " annotation with same ID already found, must resolve "
"to single target";
diag.attachNote(it->second.op->getLoc())
<< "tracker with same ID already found here";

Note that PrefixModules happens after Dedup, but it might be possible to hit a scenario where Dedup makes local annotations non-local, and that could potentially fail the single-instantiation check, which I think is what you're referring to.

I guess my thought is with this PR, we can support some local annotations end-to-end, and in the cases where it doesn't work, we'd hit a well defined error case. So, this should support some cases, but it shouldn't allow any ambiguous cases through; those should still error in the expected ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this help assuage your concerns? If the current state is fine, I'll go ahead and merge this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I have zero problems with this PR! Good to go!

I'm bringing up this problem with dedup because it's a general problem with any annotation representation. If this fix gets you further, please go for it. 👍

Annotations are generally problematic and we're slowly tackling getting off them. You're aware of the problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep makes sense, thanks. I think with this we'll at least be at parity with what we could do with the tracker annotations and EmitOMIR. Excited to design a more dataflow-y representation for paths.

12 changes: 7 additions & 5 deletions test/Dialect/FIRRTL/resolve-paths-errors.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,16 @@ firrtl.module @VectorTarget(in %a : !firrtl.vector<uint<1>, 1>) {
firrtl.circuit "AmbiguousPath" {
firrtl.module @AmbiguousPath() {
// expected-error @below {{unable to uniquely resolve target due to multiple instantiation}}
%0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousPath|Child"
%0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousPath|Child/grandchild:GrandChild"
// expected-note @below {{instance here}}
firrtl.instance child0 @Child()
// expected-note @below {{instance here}}
firrtl.instance child1 @Child()
}
firrtl.module @Child() {}
firrtl.module @Child() {
firrtl.instance grandchild @GrandChild()
}
firrtl.module @GrandChild() {}
}

// -----
Expand All @@ -102,18 +105,17 @@ firrtl.class @Test(){

firrtl.circuit "UpwardPath" {
firrtl.module @UpwardPath() {
%wire = firrtl.wire : !firrtl.uint<8>
firrtl.instance child @Child()
}

firrtl.module @Child() {
%om = firrtl.object @OM()

%wire = firrtl.wire : !firrtl.uint<8>
}

firrtl.class @OM(){
// expected-error @below {{unable to resolve path relative to owning module "Child"}}
%0 = firrtl.unresolved_path "OMReferenceTarget:~UpwardPath|UpwardPath>wire"
%0 = firrtl.unresolved_path "OMReferenceTarget:~UpwardPath|UpwardPath/child:Child>wire"
}
}

24 changes: 24 additions & 0 deletions test/Dialect/FIRRTL/resolve-paths.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,30 @@ firrtl.module @Child() {

// -----

// CHECK-LABEL: firrtl.circuit "LocalPath"
firrtl.circuit "LocalPath" {
// CHECK: firrtl.module @LocalPath()
firrtl.module @LocalPath() {
// CHECK: %0 = firrtl.path instance distinct[0]<>
%0 = firrtl.unresolved_path "OMInstanceTarget:~LocalPath|Child"

// CHECK: %1 = firrtl.path reference distinct[1]<>
%1 = firrtl.unresolved_path "OMReferenceTarget:~LocalPath|Child>wire"

// CHECK: firrtl.instance child0 @Child()
// CHECK: firrtl.instance child1 @Child()
firrtl.instance child0 @Child()
firrtl.instance child1 @Child()
}
// CHECK: firrtl.module @Child() attributes {annotations = [{class = "circt.tracker", id = distinct[0]<>}]}
firrtl.module @Child() {
// CHECK: %wire = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[1]<>}]} : !firrtl.uint<8>
%wire = firrtl.wire : !firrtl.uint<8>
}
}

// -----

// CHECK-LABEL: firrtl.circuit "PathMinimization"
firrtl.circuit "PathMinimization" {
// CHECK: firrtl.module @PathMinimization()
Expand Down
Loading