-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
[FIRRTL] LowerMemory change in #6719 leads to ambiguous targets in EmitOMIR #6830
Comments
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
It looks like by deduping the wrapper modules, we no longer uphold that constraint. |
Hey @mikeurbach, is there any update on this issue? I'd love to get #6719 re-merged if possible. |
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. |
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.
The text was updated successfully, but these errors were encountered: