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

[WIP] Implement RFC 41: Fixed point types. #1005

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zyp
Copy link
Contributor

@zyp zyp commented Dec 24, 2023

No description provided.

Copy link

codecov bot commented Dec 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (82d35fb) 86.58% compared to head (7c1d8c4) 86.58%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1005   +/-   ##
=======================================
  Coverage   86.58%   86.58%           
=======================================
  Files          41       41           
  Lines        7035     7035           
  Branches     1675     1675           
=======================================
  Hits         6091     6091           
  Misses        782      782           
  Partials      162      162           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return self._shape.signed

@ast.ValueCastable.lowermethod
def as_value(self):
Copy link

Choose a reason for hiding this comment

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

I think this should be:

    @ast.ValueCastable.lowermethod
    def as_value(self):
        if self.signed:
            return self._target.as_signed()
        return self._target

or similar, otherwise multiplying two signed fixed point numbers results in an unsigned fixed point number

Copy link

@ld-cd ld-cd Apr 5, 2024

Choose a reason for hiding this comment

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

This approach does not work, it seems to break initialization of lib.wiring wires but I'm not quite sure how

Copy link
Member

Choose a reason for hiding this comment

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

otherwise multiplying two signed fixed point numbers results in an unsigned fixed point number

Why would that be the case?

Copy link

Choose a reason for hiding this comment

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

I dug into this a bit more; I believe (with my limited understanding of the type system here), is that the signdedness of _target is given by the signdedness of the underlying raw value that the number was constructed with which is not necessarily the same as the signdedness of the fixed point type itself. __mul__ uses .as_value and then constructs the new fixed point type based on the signdedness of the result of the multiplication when it is cast back to a fixed point type:

In [19]: x = fixed.SQ(8, 8)(unsigned(17))

In [20]: x, x.as_value()
Out[20]: ((fixedpoint SQ8.8 unsigned(17)), unsigned(17))

In [21]: y = fixed.Shape.cast(x.as_value())

In [22]: y, y.signed
Out[22]: (fixed.Shape(17, 0, signed=False), False)

several of the other arithmetic operators do the same thing and this seems to break proper sign handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're not really supposed to be able to make a fixed.Value that differs in width/signedness from the underlying Value. Expect your first line to raise an error.

Copy link

Choose a reason for hiding this comment

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

Ok I've checked my notes, one place where I had a conversion to unsigned from signed was because of doing what you describe intentionally (a hacky attempt at implementing complex numbers in a struct and slicing the underlying unsigned value).

The other two I believe come from here, and here (because Cat appears to be treated as unsigned?), eg:

In [53]: x = Signal(fixed.SQ(8, 8))

In [54]: x << 9
Out[54]: (fixedpoint UQ18.0 (cat (const 1'd0) (sig x)))

In [55]: y = x.round(f_width = 9)

In [56]: y, y * y
Out[56]:
((fixedpoint SQ8.9 (cat (const 1'd0) (sig x))),
 (fixedpoint UQ18.18 (* (cat (const 1'd0) (sig x)) (cat (const 1'd0) (sig x)))))

Copy link
Member

Choose a reason for hiding this comment

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

The signedness rules are available for all of the operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants