Fix hash #355
Fix hash #355
Conversation
This commit fixes a bug where we would potentially call __call__ (where the actual uniquification logic occurs) multiple times for each definition.
Previously, we were keeping track of which definitions we had already touched in the InstanceGraphPass by using definition.__name__ as a dict key. Instead, we should now use id(definition) as multiple definitions with the same name can exist (but they have different id's).
We previously handled how we rename duplicated circuits incorrectly. This commit fixes that.
tests/test_uniquify.py
Outdated
|
|
||
|
|
||
| def test_key_error(): | ||
| import mantle |
There was a problem hiding this comment.
For some reason I'm not able to reproduce the error this test targets without these mantle elements -- I don't think the issue (and therefore this test) really has anything to do with mantle, but it is sufficiently nuanced that I couldn't reproduce in a hand-crafted way w/o mantle.
@leonardt thoughts?
There was a problem hiding this comment.
The circuit combinational tests also depend on a Mux (for testing the generated muxes from if statements). What I did was stub out the DefineMux logic without a definition. Would that be sufficient for this test? Or does it need the actual implementation? Either way, I think it might be okay to copy the Mux definition from mantle and keep it the magma test suite as a common circuit. It's not essential to keep this up to date with mantle, it's more to serve as an example of a non-trivial circuit definition.
There was a problem hiding this comment.
Here's a link to the stub logic: https://github.com/phanrahan/magma/blob/master/tests/test_circuit_def.py#L7-L31
There was a problem hiding this comment.
Thanks for the pointers @lenny. It would be good to be able to mock mantle in general.
For this test I just pretty much copied from: https://github.com/phanrahan/mantle/blob/29ec32d76b2faf189525fcb5b5a845d817dcd9b2/mantle/coreir/MUX.py#L46-L119.
There was a problem hiding this comment.
I verified that the test is "valid" in that it causes the proper failure without this change.
Essentially c/p the code from mantle for creating Mux2x6. This should be ok since it is a relatively stable component.
Pull Request Test Coverage Report for Build 1485
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 1485
💛 - Coveralls |
Addresses phanrahan/mantle#131