Skip to content
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

Keep original name for keyword argument #359

Closed
wants to merge 3 commits into from

Conversation

splhack
Copy link
Contributor

@splhack splhack commented Feb 22, 2019

I'm not sure whether there is a reason or not, but with this small example

@m.circuit.combinational
def test(a: m.Bit) -> m.Bit:
  return ~a

the argument name turns into with _0 suffix, after recent ssa change.

class test(m.Circuit):
    IO = ['a_0', m.In(m.Bit), 'O', m.Out(m.Bit)]

I assume I can use positional argument to call the combinational logic like this.

b = test(io.a)

and also keyword argument like this.

b = test(a=io.a)

but this keyword argument doesn't work due to _0 suffix. I needed to change it into test(a_0=io.a).

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1496

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 73.533%

Totals Coverage Status
Change from base Build 1493: 0.02%
Covered Lines: 3834
Relevant Lines: 5214

💛 - Coveralls

@coveralls
Copy link

coveralls commented Feb 22, 2019

Pull Request Test Coverage Report for Build 1497

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 73.533%

Totals Coverage Status
Change from base Build 1493: 0.02%
Covered Lines: 3834
Relevant Lines: 5214

💛 - Coveralls

@leonardt
Copy link
Collaborator

The reason for this change was that it was the most elegant solution to handle the case where a variable in the definition matched a variable in the argument list.

There's two ways to address this issue: One is to update the ssa algorithm to appropriately handle inputs. The other is to have circuit.combinational "wrap" the generated ssa circuit with the original port names wired up to the ssa'ed port. The second version is an easier fix that addresses this issue (at the cost of one extra "wrapper" layer introduced in the generated code, I will work on it.

@splhack
Copy link
Contributor Author

splhack commented Feb 23, 2019

fixed by #361

@splhack splhack closed this Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants