search

llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.62k stars 281 forks source link

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

Open mikeurbach opened 5 months ago

mikeurbach commented 5 months ago

In bea85107e8e42261e9c2e3ed24dd398e7f33b915 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 commented 5 months ago

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: https://github.com/llvm/circt/blob/bea85107e8e42261e9c2e3ed24dd398e7f33b915/lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp#L736-L737

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

tymcauley commented 3 months ago

Hey @mikeurbach, is there any update on this issue? I'd love to get https://github.com/llvm/circt/pull/6719 re-merged if possible.

mikeurbach commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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.