-
Notifications
You must be signed in to change notification settings - Fork 214
Add IEEE-754-Compatible Integer Rounding Primitives And Exhaustive DSLX Tests #3092
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
base: main
Are you sure you want to change the base?
Conversation
ebc903a
to
fd3324e
Compare
I don't understand why CI is failing:
this check has nothing to do with the changes made in this PR (which add a DSLX file and add to documentation). |
Supports the 5 IEEE rounding modes
fd3324e
to
2e322ea
Compare
308570b
to
e442156
Compare
Looks like this was fixed w/ rebasing? (underlying issues w/ |
@@ -0,0 +1,386 @@ | |||
"""Generate DSLX round() tests. | |||
This script produces a comprehensive set of DSLX tests that exercise the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't having C++ JIT based tests be a more scalable approach than generating DSLX code? see https://github.com/google/xls/tree/main/xls/dslx/stdlib/tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are pros and cons, for sure.
Pros of Python generating DSLX tests, and committing the generated tests:
- We don't need to worry about what C++ rational number library might or might not exist in google's 3rd party. This is especially relevant to contributors outside of google. We keep XLS build simpler.
- The generated, checked in tests now provide a test of DSLX logic in DSLX, without reference to some other programming language.
- It's done and it works.
Good enough:
- Running the tests (exhaustive for 1 through 5 bits) is fast enough, 11s on my machine
Cons:
- We have generated tests instead of something more easily editable.
- As long as we're not deeply changing DSLX syntax, or the round module's API, then I don't foresee a need to regenerate the tests any time soon.
- We could test more samples if we used JIT.
- The existing tests already cover all logic paths and corner cases, IIUC, so I don't see why more is better.
All things in engineering are tradeoffs. IMO this tradeoff is fine, but your team might have a different perspective. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to worry about what C++ rational number library might or might not exist in google's 3rd party. This is especially relevant to contributors outside of google. We keep XLS build simpler.
I don't think it implement rational numbers (nor fixed point arithmetic out of the box), but we do depend transitively on eigen
xls/xls/dslx/stdlib/tests/BUILD
Line 515 in 772e789
"@eigen//:eigen3", |
/cc @rmlarsen active maintainer of eigen and contributor to apfloat who might have opinions.
checked in tests
any reason this need to be checked-in?
Cons:
I realize this might be a somehow conservatism argument, but something that striked me as a "Cons" is that it's different from the other type of exhaustive testing that we do in the tree, if we make that tradeoff for round
maybe we should revisit C++ testing for other apfloat routines?
Curious what @cdleary and @richmckeever think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically on the topic of how to test, I believe that we should keep a test method that allows us to implement more rounding modes beyond those supported by IEEE 754. wikipedia rounding has a bunch. I could imagine later implementing round ties to zero, ties up, ties down, ties to odd, directed rounding away from zero, etc. If we rely on a testing method that locks us in to only IEEE 754 rounding modes, that's not helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on the generated tests. But I am exceedingly curious to see how using this class in apfloat would ultimately affect PPA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how using this class in apfloat would ultimately affect PPA
I did some limited testing with g8r. You only pay for what you use, as long as that is visible to XLS.
Specifically what I tested:
- define a function that always rounds 16 bits away and calls the dynamic round with that constant
- copy round's implementation, edit and make it parametric on number of bits rounded. Call this
with 16 bits rounded.
Both versions were optimized with g8r; the results were identical.
Thus, for apfloat of a specific fraction size, the user will get specialized code.
I think the same thing happens for rounding modes:
Let's say you only want 3 rounding modes. Your public API should be a new enum with only
those 3 rounding modes, and a function that takes that enum, and translates it to the
RoundingMode enum of this library. XLS optimizer will remove the unused rounding modes from
the optimized code.
So a future user of apfloat, if they only want a proper subset of rounding modes, should make it clear to the compiler that they're only using that subset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dank-openai what do think of continuing the discussion about jit wrapper vs generated tests in a separate PR and merging this w/o exhaustive testing (but w/ "some" manual #[test]
and/or #[quickcheck]
for the interesting corner cases next to the implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's awesome!
Thanks for contributing this, factoring out the rounding logic will definitly help making apfloat
more maintainable.
Asking a few preliminary questions.
Yes, it was. |
My pleasure!
I have also found this more broadly useful; I have made use of it with the fixed point module too. I'm sure others will find uses for rounding functions outside of just apfloat. |
xls/dslx/stdlib/round.x
Outdated
// round(RNE, 4 bits, unsigned, u5:0b1_1000) -> rounds up (retained msb is 1) | ||
// round(RNE, 4 bits, unsigned, u4:0b1000) -> rounds down (no retained bits) | ||
pub fn round | ||
<S: bool, N: u32, W_NBR: u32 = {std::clog2(N + u32:1)}, NP1: u32 = {N + u32:1}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we expect W_NBR
, NP1
and W_SAFE
to be overridable by the user?
If not, maybe we should just move them as const
in the function body? (since the return type doesn't depend on them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did what I could. In some cases, the type arguments are used to define argument or result types, so they cannot be made constant in the function body.
E.g. W_NBR in this case must remain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flagging this for @richmckeever, I don't think we have a good way to create depending parametrics that are not user-overridable; since we rely on default value to slot the parametric expression.
do we have a bug about this yet?
truncation paths, covering all five IEEE-754 modes and flagging overflow when the rounded integer exceeds the available width.
bits, and a generator-produced matrix of 372 tests spanning bit widths 1–5, every input value, all possible num_bits_rounded, and all rounding modes.
expected overflow behaviour.