Skip to content

Conversation

@dakersnar
Copy link
Owner

No description provided.

@dakersnar
Copy link
Owner Author

dakersnar commented Jan 23, 2023

This implementation was ported from https://www.intel.com/content/www/us/en/developer/articles/tool/intel-decimal-floating-point-math-library.html. Specifically, this is the bid32_to_binary32 implementation from bid_binarydecimal.c. Most of the other conversion methods in that file will also need to be ported. Reviews of this PR should have that implementation in mind.

Notably, I have not tested this implementation yet. Any suggestions for how to properly test this appreciated. I'm not sure how portable the testing framework Intel provides is, and I don't know what we will be able to borrow from that.

I'm probably going to work on binary32_to_bid32 next so we can at the very least have round trip tests.

@dakersnar dakersnar changed the title bid32_to_binary32 bid32_to_binary32 and binary32_to_bid32 Jan 24, 2023
@dakersnar
Copy link
Owner Author

I added the reverse direction. I'm slightly concerned at the size of the code, especially given that we haven't touched larger types such as bid128 or binary64. I would also like to note that the more we rely on this intel implementation the less we can "copy paste" for implementing Decimal64 and Deicmal128. I imagine this will cause issues when we port things like Addition, because implementing it on all three types will no longer be "free" as we imagined it being.

bid_roundbound_128[((s & 1) << 1) + (int)(c_prov & 1)].Item2, // PORT: the calculation of this index factors in the rounding mode in the original code
bid_roundbound_128[((s & 1) << 1) + (int)(c_prov & 1)].Item1,
z.Item5,
z.Item4
Copy link
Owner Author

Choose a reason for hiding this comment

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

I will also generally note that porting this code can be very error prone. I happened to discover a typo I made last week in which this line was incorrectly z.Item3 instead of z.Item4. If I hadn't found it by chance, I am not sure how we would have ever found that bug. Even with exhaustive tests (which are still up in the air) I'd imagine debugging this code is a nightmare. We should keep this in mind when thinking about development cost.

@dakersnar
Copy link
Owner Author

Notably, I have not tested this implementation yet. Any suggestions for how to properly test this appreciated. I'm not sure how portable the testing framework Intel provides is, and I don't know what we will be able to borrow from that.

I think I found what I was looking for in TESTS/readtest.in. This test data is just a subset of what they used to test the library, but it should hopefully be sufficient for our purposes. Notably I don't know what column 2 and 5 are (probably rounding and exception related?), but the numbers inside the square brackets seem to be converting correctly.

image

@dakersnar
Copy link
Owner Author

I added a couple passing test cases as examples. There are many more in the intel library that can eventually be manually ported with little difficulty.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants