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

[mikeurbach]

The existing path support was built up based on the assumption that
every target is unique. That is true for FIRRTL produced by standard
Chisel code, which elaborates unique modules for each instance. We
definitely don't want to limit ourselves to this world, and we should
support targeting things that are multiply instantiated when it is not
ambiguous what we refer to.

This patch relaxes the single-instantiation constraints for local
targets, which refer to a module or something inside a module,
regardless of how many times or at what paths that particular module
was instantiated.

This required a couple changes through the pipeline:

ResolvePaths already had an early exit for the local path case, but
this needed to come before the single-instantiation check.

LowerClasses needed a couple small changes to not enforce the
single-instantiation check in the local path case, and to build a
hierpath that just has a single element.

While this is not a new requirement, we can still get ambiguous local
targets, for instance from nested module prefixing. The error message
in LowerClasses for this case was made a little more clear.

[uenoku]

LGTM

[seldridge]

This seems fine.

This does mean that this approach can work with pre-deduplicated circuits. However, it cannot, and should not, work with circuits that go through deduplication in CIRCT (which will necessarily create non-local annotations).

If there is an expectation that this should work for both paths, then we would need to move to a representation that does not use annotations, e.g., an intrinsic.

[mikeurbach]

This does mean that this approach can work with pre-deduplicated circuits. However, it cannot, and should not, work with circuits that go through deduplication in CIRCT (which will necessarily create non-local annotations).

Hmm, I think I need to understand more about what won't work here, since my hope is to start supporting pre-deduplicated circuits in some cases.

Here's how I would represent what CIRCT main supports:

• Local targets: check the target is singly instantiated and uniquely identifiable, make a non-local annotation, and the rest of the pipeline handles it
• Non-local targets: check the target is singly instantiated and uniquely identifiable, make a non-local annotation, and the rest of the pipeline handles it

This works for both cases in practice because standard Chisel produces duplicated circuits in the FIRRTL. If the FIRRTL is pre-deduplicated, the local target case can fail immediately in ResolvePaths.

What I'm trying to change here is the Local targets case--rather than checking the target is singly instantiated and making a non-local annotation, it makes a local annotation. This is intended to be similar to how references worked with the original EmitOMIR flow--if something like dedup needs to turn a local annotation into a non-local annotation, the pass is free to do that, and the rest of the pipeline should handle it or error out in the face of ambiguity, similarly to what EmitOMIR does, but using a slightly different mechanism with distinct attributes.

This is intended to make some local targets now possible, for the case where the FIRRTL comes pre-deduplicated, and those targets don't become ambiguous at some point in the pipeline. There are still cases where we wouldn't fail at ResolvePaths, but transformations would lead to a failure later (with a decent error message).

Maybe I'm missing something, but can you share more about what can't work? The existing path support was built up based on the same OMIRTrackerAnnotations we previously used with EmitOMIR, and can handle both local and non-local annotations. This patch is just allowing local annotations to be used directly when requested from the input, and letting the later stages of the pipeline ensure they are unambiguous after transforms like PrefixModules and Dedup.

[mikeurbach]

we would need to move to a representation that does not use annotations, e.g., an intrinsic.

And yes, I think we do need to move to a different representation in the long term, but we went the route of reusing the OMIRTrackerAnnotations when we introduced paths to FIRRTL so we could resolve them into something the rest of the pipeline already works with in ResolvePaths. So this PR is about allowing us to resolve local annotations, which the existing flow should support (or give an error if it can't). I do want to come up with a better representation, but we'll need to teach lots of passes that currently work with annotations to keep that representation up to date.

[seldridge]

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.

[mikeurbach]

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.

[mikeurbach]

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

[seldridge]

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.

[mikeurbach]

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.