google / google/xls

Consolidate apfloat "round with ties to nearest even" logic

Open
#1,656 0 comments 0 reactions 0 assignees View on GitHub
cleanup dslx
Dominant language
C++
Stars
1.9k
Forks
283
Avg merge
2d 10h
Merged PRs (30d)
135

Description

During code review of https://github.com/google/xls/commit/f8853882164c629c39aa3fb75336d4d7615d1729, we discussed that there would now be multiple (different) implementations of "should we round up (with ties to nearest even)" for apfloats. Specifically, we have:

https://github.com/google/xls/blob/53f30d068f9bb9e7888ecbe4c34273f5c3223e27/xls/dslx/stdlib/apfloat.x#L280-L298

https://github.com/google/xls/blob/53f30d068f9bb9e7888ecbe4c34273f5c3223e27/xls/dslx/stdlib/apfloat.x#L794-L806

We left it as is because of a few considerations:

1. The implementations are different, and it would be nice to compare QoR.
2. Neither implementation is part of the public API.
3. `rne` (unintentionally?) adds behavior for `lsb_idx` overflowing the input fraction size (https://github.com/google/xls/blob/53f30d068f9bb9e7888ecbe4c34273f5c3223e27/xls/dslx/stdlib/apfloat.x#L311), while the latter knows this will never happen by construction.
4. `rne` may benefit from renaming since it doesn't do any "rounding" as implied by the name.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.