Skip to content

Enforce Unique Names #496

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enforce Unique Names #496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions magma/circuit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,8 +596,12 @@ def DefineCircuit(name, *decl, **args):
currentDefinition = DefineCircuitKind( name, (Circuit,), dct)
return currentDefinition

__seenCircuitNames = {}
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

debug_info = get_callee_frame_info()
currentDefinition.end_circuit_filename = debug_info[0]
currentDefinition.end_circuit_lineno = debug_info[1]
Expand Down
3 changes: 1 addition & 2 deletions magma/syntax/combinational.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ def wrapped(fn):
else:
wrapped_combinational = ast_utils.inspect_enclosing_env(
_combinational,
decorators=[combinational],
st=env
decorators=[combinational]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I guess I didn't realize I had to run pip upgrade, but that makes sense.

)
return wrapped_combinational(fn)
10 changes: 5 additions & 5 deletions tests/test_circuit/test_define.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def test_for_loop_def(target, suffix):
And2 = m.DeclareCircuit('And2', "I0", m.In(m.Bit), "I1", m.In(m.Bit),
"O", m.Out(m.Bit))

main = m.DefineCircuit("main", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))
main = m.DefineCircuit("main2", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))

and2_prev = None
for i in range(0, 4):
Expand Down Expand Up @@ -86,7 +86,7 @@ def test_interleaved_instance_wiring(target, suffix):
And2 = m.DeclareCircuit('And2', "I0", m.In(m.Bit), "I1", m.In(m.Bit),
"O", m.Out(m.Bit))

main = m.DefineCircuit("main", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))
main = m.DefineCircuit("main3", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))

and2_0 = And2()
and2_1 = And2()
Expand Down Expand Up @@ -115,7 +115,7 @@ def test_unwired_ports_warnings(caplog):
And2 = m.DeclareCircuit('And2', "I0", m.In(m.Bit), "I1", m.In(m.Bit),
"O", m.Out(m.Bit))

main = m.DefineCircuit("main", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))
main = m.DefineCircuit("main4", "I", m.In(m.Bits[2]), "O", m.Out(m.Bit))

and2 = And2()

Expand All @@ -134,7 +134,7 @@ def test_2d_array_error(caplog):
And2 = m.DeclareCircuit('And2', "I0", m.In(m.Bit), "I1", m.In(m.Bit),
"O", m.Out(m.Bit))

main = m.DefineCircuit("main", "I", m.In(m.Array[2, m.Array[3, m.Bit]]),
main = m.DefineCircuit("main5", "I", m.In(m.Array[2, m.Array[3, m.Bit]]),
"O", m.Out(m.Bit))

and2 = And2()
Expand All @@ -157,7 +157,7 @@ def test_anon_value(target, suffix, T):
And2 = m.DeclareCircuit('And2', "I0", m.In(T), "I1", m.In(T),
"O", m.Out(T))

main = m.DefineCircuit("main", "I0", m.In(T), "I1", m.In(T),
main = m.DefineCircuit(f"main6_{target}_{suffix}_{T}", "I0", m.In(T), "I1", m.In(T),
"O", m.Out(T))

and2 = And2()
Expand Down