Hi Jonathan, The issue is that I didn't cast __dp.count() to uint32_t:
- const auto __r0 = __dp.count() + __r2_e3; + const auto __r0 = static_cast<uint32_t>(__dp.count()) + __r2_e3; The above would be a better fix. Indeed, __r0 belongs to [0, 2^32[ which allows all arithmetics that follow to be performed on uint32_t values. For performance this is better than using signed integers. I wrongly assumed __dp.count() was int32_t which would make __r0 also uint32_t. It turns out, it is int64_t so are __r0 and other expressions including __y1 + __z2. Hence, we're losing a bit of performance. I'm glad the compiler warned us. (Although I don't understand why I didn't get the warning.) Thanks, Cassio. On Thu, Feb 25, 2021 at 11:56 AM Jonathan Wakely <jwak...@redhat.com> wrote: > > On 24/02/21 17:28 +0000, Jonathan Wakely wrote: > >On 23/02/21 13:24 +0000, Cassio Neri via Libstdc++ wrote: > >>This patch reimplements std::chrono::year_month_day::_S_from_days() which > >>retrieves a date from the number of elapsed days since 1970/01/01. The new > >>implementation is based on Proposition 6.3 of Neri and Schneider, "Euclidean > >>Affine Functions and Applications to Calendar Algorithms" available at > >>https://arxiv.org/abs/2102.06959. > >> > >>The aforementioned paper benchmarks the implementation against several > >>counterparts, including libc++'s (which is identical to the current > >>implementation). The results, shown in Figure 4, indicate the new > >>algorithm is > >>2.2 times faster than the current one. > >> > >>The patch adds a test which loops through all integers in [-12687428, > >>11248737], > >>and for each of them, gets the corresponding date and compares the result > >>against its expected value. The latter is calculated using a much simpler > >>and > >>easy to understand algorithm but which is also much slower. > >> > >>The interval used in the test covers the full range of values for which a > >>roundtrip must work [time.cal.ymd.members]. Despite its completeness the > >>test > >>runs in a matter of seconds. > >> > >>libstdc++-v3/ChangeLog: > >> > >> * include/std/chrono: > >> * testsuite/std/time/year_month_day/3.cc: New test. > > > >Thanks! I'm committing this to trunk (it only changes new C++20 > >material so OK during stage 4 ... and anyway it's both faster and > >better tested than the old code). > > > >I've tweaked it slightly to keep some lines below 80 columns, but no > >changes except whitespace. > > I've made the attached tweak, to avoid a narrowing conversion from > long to int. Tested x86_64-linux, committed to trunk. >
