Enforce Unique Names #496
Enforce Unique Names #496
Conversation
|
Also @leonardt there was a breaking change to the combinational file on my machine which I fixed (?) here. |
| @@ -275,7 +275,6 @@ def wrapped(fn): | |||
| else: | |||
| wrapped_combinational = ast_utils.inspect_enclosing_env( | |||
| _combinational, | |||
| decorators=[combinational], | |||
| st=env | |||
| decorators=[combinational] | |||
There was a problem hiding this comment.
Can you try updating to the latest version of ast_tools and reverting this change? (pip install --upgrade ast_tools), the st argument was introduced in the latest release.
There was a problem hiding this comment.
Ok I guess I didn't realize I had to run pip upgrade, but that makes sense.
| def EndDefine(): | ||
| if currentDefinition: | ||
| if currentDefinition.name in __seenCircuitNames: | ||
| raise Exception(f"The circuit name '{currentDefinition.name}' was already used. Module names must be unique!") | ||
| __seenCircuitNames[currentDefinition.name] = 1 |
There was a problem hiding this comment.
For now, could we add this as a configurable option? That way we can merge it into the mainline release ASAP but then figure out how exactly we want to handle it and what the default behavior should be (I expect there to be some discussion).
We could add a simple API like set/get_debug_mode https://github.com/phanrahan/magma/blob/master/magma/config.py#L14-L24 to enable this?
There was a problem hiding this comment.
IMO I would rather have the discussion and decide to fix the issue or not. We don't need to have this merged in immediately. What do people think? Are there any arguments on the other side?
|
This issue has ping-ponged a bit in magma previously. I think this condition used to be enforced, but we changed it because not enforcing it was more general and robust. In particular, if you have a top level design that's using components from disparate places, they may not have unique names. And in fact they may not be modifiable (imagine you're importing some third party magma code or something). Plus, the issue of not caching generators is separate and should be solved "directly". I think one reasonable thing would be to make auto-uniquification off by default in the compiler; i.e. this error is raised at compile time rather than define time. This mode already exists, it just has to be enabled. |
|
@rsetaluri I think that would help with performance (avoiding the repr uniquification time), another incremental option might be to allow selective whitelist of names to uniquify (so if there's a commonly used name such as |
|
Another improvement would be to add automatic name generation in the |
I suggest we change magma to enforce that module names are unique at definition time. Currently, if the user forgets to memoize a generator, their code will create thousands of duplicate module definitions, which the compiler will silently accept. This is causing us to have hour long compile times, and it's not possible to debug this if we don't raise an error.
I added a quick fix to enforce this behavior, but (if we agree this is the behavior we want), I will need help squashing all the duplicate name bugs. It looks like a large number of generators in the repo that are not being memoized.