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

Jump to bottom

hdl.ast.Value: Add .to_signed() method #481

Closed
wants to merge 1 commit into from
Closed

hdl.ast.Value: Add .to_signed() method #481

wants to merge 1 commit into from

Conversation

pbsds
Copy link

@pbsds pbsds commented Aug 19, 2020

We currently have a .as_unsigned(), but if the MSB is set
then its numerical value will be changed by it.

Consider this case for the unsigned variables a and b of same width, where a < b:

a - b               # will underflow
a.as_unsigned() - b # value of a is changed if a[-1] == 1
a.to_unsigned() - b # correct

@pbsds
hdl.ast.Value: Add .to_signed() method
e12bf90 
We currently have a .as_unsigned(), but if the MSB is set
then its numerical value will be changed by it.

Consider this case for unsigned variables a and b of same width, where a < b:

    a - b               # will underflow
    a.as_unsigned() - b # value of a changed if a[-1] == 1
    a.to_unsigned() - b # correct
@whitequark
Copy link
Member

Sorry it took so long to get back to you!

a - b               # will underflow
a.as_unsigned() - b # value of a is changed if a[-1] == 1
a.to_unsigned() - b # correct

I assume you mean the following:

a.to_signed() - b # correct

We discussed your proposal on the last meeting. I agree that it addresses an issue with nMigen language as it exists. However, I have two concerns:

  • It would be very easy to confuse as_signed (whose function is already not entirely obvious) with to_signed (which differs in a subtle way).
  • The way to_signed works is a subset of how a shape conversion operator would work, i.e. Proposal 1 from [RFC] Shape Conversion Operators Round 2 #464 would make it unnecessary.

Because of these concerns I think the issue you are highlighting would be best addressed as a part of #464.

@whitequark whitequark closed this Oct 26, 2020
@whitequark whitequark mentioned this pull request Oct 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pbsds @whitequark