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

Refactor implicit coercion logic #855

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

leonardt
Copy link
Collaborator

Provides reuseable decorator and function to handle magma's implicit
coercion of Python values.

For now, I moved the logic from wiring and mux to a shared place to
ensure consistency. There's probably other places to do this, but we
can do this on demand as we enounter incosistencies.

One issue is that int coercion can't really happen up front like this
since it is determined by the place it's used (i.e. when it's wired to a
Bits we check it can fit in the bit width). @rsetaluri perhaps we could
use SmartBits for this? (convert it to a smartbits that's automatically
extended to wire to a regular bits, but I think right now our rules
don't allow implicit truncation, but we could define a special SmartBits
subclass that only extends. My one concern would be whether this would
make the error a bit harder to traceback)

Fixes #828

Provides reuseable decorator and function to handle magma's implicit
coercion of Python values.

For now, I moved the logic from wiring and mux to a shared place to
ensure consistency.  There's probably other places to do this, but we
can do this on demand as we enounter incosistencies.

One issue is that `int` coercion can't really happen up front like this
since it is determined by the place it's used (i.e. when it's wired to a
Bits we check it can fit in the bit width).  @rsetaluri perhaps we could
use SmartBits for this? (convert it to a smartbits that's automatically
extended to wire to a regular bits, but I think right now our rules
don't allow implicit truncation, but we could define a special SmartBits
subclass that only extends.  My one concern would be whether this would
make the error a bit harder to traceback)

Fixes #828
@leonardt
Copy link
Collaborator Author

I think if we used SmartBits to coerce integers, we could solve this issue: #855 too by just converting them to SmartBits and having the mux primitive manage the required extension to match the value the mux output is wired too. But, @rsetaluri, this would require SmartBits to work with the mux primitive. We could build this in since mux is a primitive, but I wonder if there's a more general pattern we might consider to allow circuits/generators to support/extend SmartBits?

@leonardt
Copy link
Collaborator Author

Bump @rsetaluri

I'd like to use this as a starting point for fixing #969. Basically, I'd like to add the rule that we shouldn't truncate during conversion between python integers to magma values. I think this decorator would be a good place to do it and we should have all the operators reuse that logic rather than adding it to them individually.

@leonardt
Copy link
Collaborator Author

Actually scratch that, we can lower the priority but this would be a good change to get in since it's pretty straightforward and does avoid a circular import. I was able to resolve #969 inside the Array constructor logic which actually seems like a good place for it.

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.

Implicit coercion decorator for Python values
1 participant