[FIRRTL] LowerMemory change in #6719 leads to ambiguous targets in EmitOMIR

[mikeurbach]

In bea8510 we had to revert #6719. This change caused some memories that were previously targeted from OMIR to fail in the EmitOMIR pass. Opening this issue for tracking the effort to re-land the change, and will attempt to reduce the problem with a FIRRTL example here.

[mikeurbach]

I've updated the title and description. I still haven't been able to come up with a simple reproducer, but the gist of the problem is we have OMIR targeting the memories, and by the time we get to EmitOMIR, we expect a single path through the instance hierarchy to each memory:

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

Lines 736 to 737 in bea8510

	auto diag = tracker.op->emitError("OMIR node targets ambiguous component `")
	<< opName.getValue() << "`";

It looks like by deduping the wrapper modules, we no longer uphold that constraint.

[tymcauley]

Hey @mikeurbach, is there any update on this issue? I'd love to get #6719 re-merged if possible.

[mikeurbach]

I don't think anyone has been looking at addressing this. I could reproduce the issue on an internal design, but I never got around to writing a small FIRRTL reproducer based on the observations above. It should be pretty easy to write a test case that dedupes without the change in #6719 , but then fails to dedupe after it. We should include such a test case before we re-land this change.

[tymcauley]

I don't think anyone has been looking at addressing this. I could reproduce the issue on an internal design, but I never got around to writing a small FIRRTL reproducer based on the observations above. It should be pretty easy to write a test case that dedupes without the change in #6719 , but then fails to dedupe after it. We should include such a test case before we re-land this change.

I'd love to get that PR re-landed, do you have any examples of designs that expose this issue?

[mikeurbach]

Unfortunately nothing open source I can share. I never reduced a minimal example, but what we need is a test case where we have some OMIR annotations targeting memories that dedupe. With CIRCT main, they dedupe and EmitOMIR is happy, but with the change in #6719 as-is, the test case should fail with the "OMIR node targets ambiguous component" error.