
First off I'd like to thank Matt and Chris for submitting the library
for review, and for all the reviewers who submitted comments. My
apologies for not posting this sooner, but this has been a difficult
review to reach a definite result on.
First the headline result: the result of the review is INDETERMINATE.
What this means in practice: I'd like to encourage Matt and Chris to
come back for a mini-review at a later date which should focus
exclusively on the open issues listed below. I forget which library we
applied this to in the past, but I believe the result is not without
precedent.
I note that a greater weight of reviews, and/or a clearer path forward
may well have tipped the balance here. And of course as someone who's
worked with Matt and Chris, I may well be being over-harsh in order to
avoid bias... or perhaps I'm just over thinking the whole thing...
In my notes below, I've split my comments into:
* "Settled Issues", these were issues that were raised at some point but
found not to be true issues.
* "Todo", straightforward usability and implementation issues such as we
get from any review process. I've filed matching GitHub issues for each
of these.
* "Open", these are the harder knotty problems, where it's not clear
(yet) what the solution is.
RATIONALE: we had five reviews (4 public, one direct to me), of which 4
were generally positive, and one was for reject. In addition, some of
the positive reviews were directly contradictory in the direction they
thought the library should progress in ("we should only support the fast
types", vs "we should only support the fixed width types" etc). There
were also some conditions for acceptance - mostly performance based -
where it's not clear whether the objectives are possible in a sane
time-frame. For example the often referenced Intel C library is frankly
huge (source files > 6Mb in size) and makes heavy use of the "every
function is a macro" C idiom, which at least for me renders the code
virtually unreadable. As things stand I consider it unreasonable to ask
the authors to replicate that work. In late breaking news, I also hear
from Matt that GCC and IBM both "cheat" by using (mutually incompatible)
non-IEEE binary encodings.
There is also the "do we actually need decimal arithmetic" question - it
is clear that many people think we do - there have been repeated
attempts from researchers at IBM, Intel and others to push this towards
standardization, in addition to some (rather patchy) hardware support.
It's in C23, which means eventually C++ compiler support will be
required too. Oh and python has a decimal library. However the authors
need to articulate the case better - the killer application seems to be
exact round tripping from human-readable and machine representations (ie
exact representation of human data input).
John Maddock (Decimal Library Review Manager)
OPEN ISSUES
~~~~~~~~~~~
The case for decimal floating point needs to be better made - despite
support from IBM and Intel, plus repeated standardization efforts, true
killer examples seem to be thin on the ground. Perhaps liaison with
other experts in the field would be helpful here (I note that Robert
Klarer at IBM and also once around these parts has been involved in
decimal proposals,
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1977.html)?
Should arithmetic operations always be performed on the performance
optimized forms? If so, perhaps storage optimized forms should have no
operators (or their operators return the unpacked versions)? Review
managers note: can we use DEC_EVAL_METHOD to set behavior here?
How do we address the performance gap? Add the option to wrap the Intel
library? Rewrite using the same methodology as Intel (this looks like a
truly massive amount of work)? Several reviewers make this a condition
of acceptance however.
Should we use _fast integers or width specific ones? The answer appears
to be platform specific. Perhaps it's a question of changing the
correct instances of _fast integers rather than all of them?
Should we abolish the _fast types and only provide fixed-width std
compliant types: this is the forward looking option in line with what
the standards are doing... but it directly contradicts the requests of
some other reviewers.
SETTLED ISSUES
~~~~~~~~~~~~~~
The decimal32 decimal64 names etc are reserved by various standards
especially IEEE-754, we need to stick to those names.
Over long literals are acceptable without warning or error, though a
static warning facility would be nice.
One reviewer suggests removing BOOST_DECIMAL_FAST_MATH, however as it
performs similar optimizations to other libraries / compiler options, it
looks reasonable to stay if the authors wish.
One reviewer suggested shortening the namespace names - this is a tricky
one, I see the issue, but we have a tradition (rightly or wrongly) for
long_descriptive_names in boost. Plus there are always using
declarations and/or namespace aliases. I will leave this to the authors
to decide.
long/int overloads of scalbln are fine by me as these are std mandated,
however the reviewer makes a valid point that these are useful only on
16 bit platforms (I do note that the library is intended to be
micro-controller compatible though, so this may well be an issue there).
One reviewer suggests passing arguments by value is a mistake especially
for 128 bit types, other reviewers refuted this. Other than some
experimentation / double checking on windows and non-Intel platforms
this does not appear to be an issue in practice. Suggest no change
unless the double checks prove otherwise, a comment in the source or the
design rationale may well be welcome.
One reviewer suggested using CLZ in num_digits, but this has been
checked and is slower (a comment in the source to that effect would be
welcome. Note that std::lower_bound is not constexpr until C++26.
REQUESTED CHANGES
~~~~~~~~~~~~~~~~~
Literals should be in their own namespace.
https://github.com/cppalliance/decimal/issues/827
Allow implicit conversions for all conversions which do not lose
precision: https://github.com/cppalliance/decimal/issues/828
Remove sprint support. https://github.com/cppalliance/decimal/issues/829
Split into "use what you need" smaller headers, in particular iostream
and std lib support code should be in separate optional headers rather
than in #ifdef blocks. https://github.com/cppalliance/decimal/issues/830
I'm also not convinced about the value added by the other cstdio
functions, namely printf, fprintf and snprintf. They only support
decimal arguments. This means that printf("Value for id=%s is %H", id,
value) is an error if id is a const char*. Integers work because they
are cast to decimal types, which I found surprising, too. Given an int
i, printf("Value for %x is %H", i, value) works but doesn't apply the
hex specifier. The integer is promoted to a decimal type, which is
then printed. I'd have expected the latter to be a compile-time error.
I'd say that this will be the case if you replace std::common_type by
std::conjunction as Peter suggested.
https://github.com/cppalliance/decimal/issues/831
Are concept names supposed to be public? If they are, please
document them as regular entities. If they are not, please move them
into the detail namespace. https://github.com/cppalliance/decimal/issues/832
Collected documentation improvements:
https://github.com/cppalliance/decimal/issues/833
Do the exponent_type typedefs need to be public? Do they need to be 64
bit? I suspect the latter is all about extracting the bits from the
representation, and the former is a convenience to avoid a long list of
friends - although the list doesn't look *that* long? If they remain
public then a comment in the source with rationale would suffice.
https://github.com/cppalliance/decimal/issues/834
Can we add (possibly explicit) constexpr construction from string
literal? https://github.com/cppalliance/decimal/issues/835
There is an undocumented public constructor: constexpr decimal32(bool
coeff, T exp, bool sign = false) noexcept; which appears to be a
mistaken addition? https://github.com/cppalliance/decimal/issues/836
I wonder if constexpr decimal32(T coeff, T2 exp, bool sign = false)
noexcept; is under-constrained? The sign argument appears to be
superfluous given that coeff can be negative, and we know up front the
size of integer required to represent the coefficient and exponent so we
might as well make this a non-template: smaller integer arguments will
get promoted and larger arguments will quite correctly issue a warning
(I wouldn't use a type smaller than int though, otherwise the things get
altogether too picky): https://github.com/cppalliance/decimal/issues/837
There are a few places where pow10 is used with a constexpr argument,
but while the function is constexpr it is not used in a constexpr
context. The example picked up from the review was auto res_sig
{(static_cast

John Maddock wrote:
In late breaking news, I also hear from Matt that GCC and IBM both "cheat" by using (mutually incompatible) non-IEEE binary encodings.
Are these documented anywhere? It would be interesting to look at them for inspiration.
I wish. They docs just say BID format. Here is what IEEE 754 specifies: https://github.com/cppalliance/decimal/blob/develop/include/boost/decimal/de... Here is what I gathered from trying to wrap the builtin libstdc++ (IBM) types: https://github.com/cppalliance/decimal/pull/823/files#diff-dacda7fd3b3e5817b... I kept dumping the bits of powers of two etc to figure it out: https://github.com/cppalliance/decimal/pull/823/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=No+extension&show-viewed-files=true#diff-b439f27c2dfc48ec83a0f53ee47db4b27c21f2edf8ff6245931c09b06477a80dR59 Basically, it's laid out like a binary float most of the time. In Intel's case for implementation of mathematical functions they convert the Decimal32 to a float, perform an operation, and then convert back. Makes things pretty quick when your conversion operation is NOP, and then you can offload computations to presumably the MKL. Matt

On Sat, Feb 1, 2025 at 1:36 PM John Maddock wrote:
Thank you John for managing the review, and thank you to Matt and Chris for your (clearly very hard) work on the library. I am looking forward to a future revision of the library that I could be enthusiastic about, and that would grace Boost by its inclusion. Glen

>> First off I'd like to thank Matt and Chris for submitting the library>> for review, and for all the reviewers who submitted comments. My>> apologies for not posting this sooner, but this has been a difficult>> review to reach a definite result on.>> >> First the headline result: the>> result of the review is INDETERMINATE.>> Thanks John, Matt an all thereviewers. Indeed there was arich amount of commenting. Yes INDETERMINATE is the consensus,or better said the lack thereof.I also garnered an indeterminatestate from this and I agree. > Thanks for managing the review John.> We appreciate the vast number of> comments and issues you consolidated> for us. Chris and I will regroup,> and work our way down the list. Indeed, John, this was a real zinger.Thank you for your patience andtechnical guidance, which hasbeen and continues to be profoundlyexcellent. Thanks Matt for pioneering this thing. So here is my personal(very very very subjective) rundown. We will be able to* Yes, get all the syntactical comments.* Yes agree on docs and practical usage.* We will get some perf improvements, whereby I'm skeptical if these will vastly exceed 10-20%.* We might be able to wrap 1 non-conformant library, but Matt is guiding the way on this. NO: We will NOT be able to* No, we will not be able to meet the factor of 10 speed exhibited by one particular architecture-specific library. - Chris On Saturday, February 1, 2025 at 08:44:06 PM GMT+1, Matt Borlandwrote: > > First off I'd like to thank Matt and Chris for submitting the library > for review, and for all the reviewers who submitted comments. My > apologies for not posting this sooner, but this has been a difficult > review to reach a definite result on. > > First the headline result: the result of the review is INDETERMINATE. > Thanks for managing the review John. We appreciate the vast number of comments and issues you consolidated for us. Chris and I will regroup, and work our way down the list. Matt

Christopher Kormanyos wrote:
NO: We will NOT be able to* No, we will not be able to meet the factor of 10 speed exhibited by one particular architecture-specific library.
I must be missing something here, because when I look at https://www.netlib.org/misc/intel/ which is linked from https://www.intel.com/content/www/us/en/developer/articles/tool/intel-decima... I don't see anything architecture-specific in the library, everything looks like portable C. I'm also not sure why this library would be so much faster, because the number format seems to be standard IEEE BID https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc... https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc... and when I look at the comparison https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc... I see nothing that could possibly explain an order of magnitude difference in performance. Am I looking at the wrong thing?

>> NO: We will NOT be able to* No,>> we will not be able to meet the>> factor of 10 speed exhibited by>> one particular architecture-specific>> library. > I must be missing something here,> because when I look at No Peter, you are not missinganything. It is I who lackedclarity-of response. I should be more clear and thisis a personal response. Thishas not been filtered through Matt. I, known as Chris, do not identifymaximum performance as a top-levelrequirement for an IEEE-754 conformingdecimal. I am fascinated by thepotential in language andherenceand potential specification. I will not try to reproduce thatlibrary's magnificent performance,as this holds no technical interestfor me whatsoever. This might be a stark answerto your query, but I'd betterbe clear about that up front.I won't spend much time on that. Maybe a wrapper of sime othertype like we do in Multiprecisioncould give wou the "speed that you need" - Chris On Saturday, February 1, 2025 at 09:22:54 PM GMT+1, Peter Dimov via Boostwrote: Christopher Kormanyos wrote: > NO: We will NOT be able to* No, we will not be able to meet the factor of 10 > speed exhibited by one particular architecture-specific library. I must be missing something here, because when I look at https://www.netlib.org/misc/intel/ which is linked from https://www.intel.com/content/www/us/en/developer/articles/tool/intel-decimal-floating-point-math-library.html I don't see anything architecture-specific in the library, everything looks like portable C. I'm also not sure why this library would be so much faster, because the number format seems to be standard IEEE BID https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc871e673d561a03da9/LIBRARY/src/bid_internal.h#L2263-L2297 https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc871e673d561a03da9/LIBRARY/src/bid_internal.h#L772-L783 and when I look at the comparison https://github.com/featuremine/IntelRDFPMathLib/blob/edda5420528167b14b775cc871e673d561a03da9/LIBRARY/src/bid32_compare.c#L37 I see nothing that could possibly explain an order of magnitude difference in performance. Am I looking at the wrong thing? _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On Sat, Feb 1, 2025 at 7:36 PM John Maddock via Boost
Big thank you to you and the authors for the work.
SETTLED ISSUES ~~~~~~~~~~
For clz I would be interested to see the benchmarks since I know some famous libraries used the clz trick so I would be curios to learn how it can be slower. That one reviewer forgot that lower_bound is not constexpr in C++14, my apologies. But it is constexpr in C++20, not C++26 so one more reason for authors to bump the required version. ;) https://godbolt.org/z/qf8GE8WYv
participants (6)
-
Christopher Kormanyos
-
Glen Fernandes
-
Ivan Matek
-
John Maddock
-
Matt Borland
-
Peter Dimov