Skip to content

Do something with Value.implies #1239

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

Closed
whitequark opened this issue Mar 25, 2024 · 2 comments · Fixed by #1392
Closed

Do something with Value.implies #1239

whitequark opened this issue Mar 25, 2024 · 2 comments · Fixed by #1392
Milestone

Comments

@whitequark
Copy link
Member

whitequark commented Mar 25, 2024

Value.implies is a rarely used and not well-known counterpart to __or__, __xor__, etc operators. It is defined as:

    def implies(premise, conclusion):
        return ~premise | conclusion

It was originally added to aid in the development of formal testbenches, but is obviously usable in any code:

m.d.comb += Assert((self.depth > 0).implies(~empty))

The problem with this operator is that it's bitwise and sign-extending, without warning (simply because the underlying logical operators are). This is extremely weird and it's difficult to see where this behavior would be desirable. In short, if premise and conclusion are both multi-bit, then the result is a logical OR of the arrow operator applied pairwise to each bit of the premise and conclusion.


What should we do with it? Options:

  1. Deprecate and remove
  2. Deprecate use with signed operands (even though x | y or x ^ y all accept signed operands)
  3. Deprecate use with unequal length operands but allow signed operands
  4. Cast both operands with .bool() like return ~premise.bool() | conclusion.bool()
    • This is a hard compatibility break, though we can probably do a deprecation cycle?
  5. Something else
@whitequark
Copy link
Member Author

We have discussed this issue on the 2024-04-15 core subsystem meeting. Only options (1) and (4) were considered by anyone present, and there weren't compelling reasons to pick option (4). As such Value.implies will be deprecated and removed.

@wanda-phi
Copy link
Member

Deprecated in #1337.

@whitequark whitequark modified the milestones: 0.5, 0.6 May 7, 2024
wanda-phi added a commit to wanda-phi/amaranth that referenced this issue Jun 14, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants