This is an archive of the discontinued LLVM Phabricator instance.

[libc] add division, modulo, and power to UInt
ClosedPublic

Authored by michaelrj on Aug 18 2022, 3:52 PM.

Details

Summary

This adds division and power implementations to UInt. Modulo and
division are handled by the same function. These are necessary for some
higher order mathematics, often involving large floating point numbers.

Diff Detail

Event Timeline

michaelrj created this revision.Aug 18 2022, 3:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 18 2022, 3:52 PM
michaelrj requested review of this revision.Aug 18 2022, 3:52 PM
sivachandra added inline comments.Aug 19 2022, 1:35 AM
libc/src/__support/CPP/UInt.h
41 ↗(On Diff #453822)

Not sure if there is a need to initialize from a bigger array. If not, can we restrict this constructor with enable_if?

42 ↗(On Diff #453822)

Is the ternary operation correct?

81 ↗(On Diff #453822)

Instead of a static_assert, can we use enable_if to restrict this overload?

279 ↗(On Diff #453822)

cpp::optional?

283 ↗(On Diff #453822)

Naming style.

286–287 ↗(On Diff #453822)

This expression is a bit complicated to parse. Can you put it in a lambda function?

lntue added inline comments.Aug 19 2022, 8:01 AM
libc/src/__support/CPP/UInt.h
41 ↗(On Diff #453822)

I think a better option that you're looking for is an implicit constructors / assignment from UInt<M> to UInt<N> with M <= N

286–287 ↗(On Diff #453822)

Will it be easier if we implement clz for UInt<N>? So that we can just make a single shift?

michaelrj updated this revision to Diff 454619.Aug 22 2022, 2:52 PM
michaelrj marked 8 inline comments as done.

Address comments and add mod and exp functions. Still need to add tests for those last two.

libc/src/__support/CPP/UInt.h
41 ↗(On Diff #453822)

Re: sivachandra, we can't use enable_if here because typetraits.h depends on UInt.h.
Re: lntue: I've added that as well, since it's also useful, but I need the ability to initialize from a C array as well.

42 ↗(On Diff #453822)

oops, flipped.

81 ↗(On Diff #453822)

No. TypeTraits depends on UInt, so UInt can't use anything from TypeTraits. This includes enable_if.

michaelrj updated this revision to Diff 454620.Aug 22 2022, 2:56 PM

fix missing dereference in modulo

sivachandra added inline comments.Aug 23 2022, 1:46 AM
libc/src/__support/CPP/UInt.h
236 ↗(On Diff #454620)

What is the use case of detecting the overflow?

293 ↗(On Diff #454620)

Should we implement operator==(uint64_t) to make this cleaner like: other == uint64_t(1)?

297 ↗(On Diff #454620)

You should be able to do return cpp::nullopt;

338 ↗(On Diff #454620)
libc/test/src/__support/uint128_test.cpp
160

We should include them in this patch? Also, if multiplication overflow is required, we should test for that also?

sivachandra added inline comments.Aug 23 2022, 1:52 AM
libc/src/__support/CPP/UInt.h
41 ↗(On Diff #453822)

Considering that we are moving to make the CPP directory a strict subset of the C++ standard library, we should move the is_integral specialization for UInt to this file or may be to UInt128.h.

michaelrj updated this revision to Diff 456467.Aug 29 2022, 2:57 PM
michaelrj marked 6 inline comments as done.

rebase, add tests for mod and exp, general cleanup

michaelrj added inline comments.Aug 29 2022, 2:57 PM
libc/src/__support/CPP/UInt.h
236 ↗(On Diff #454620)

I used it in my initial division implementation, but I don't have a use for it now, so I've removed it.

293 ↗(On Diff #454620)

It appears that's unnecessary. I just assumed it'd need the explicit typing, but it will apparently get promoted implicitly just fine.

338 ↗(On Diff #454620)

that's in FPUtil, which depends on UInt. I will move that out in a followup patch, since it doesn't need to be in FPUtil.

sivachandra accepted this revision.Aug 30 2022, 10:19 AM

OK from my side. Please wait for @lntue.

This revision is now accepted and ready to land.Aug 30 2022, 10:19 AM
lntue accepted this revision.Aug 30 2022, 9:47 PM
lntue added inline comments.
libc/src/__support/UInt.h
252

I think the conventional name for this function is pown or pow_n. exp name is kind of reserved for e^x function.

261

Can this be replaced with cur_power *= cur_power?

320–325

Since you already included builtin_wrappers.h here, this part can be reduced to

leading_zeroes += fputil::unsafe_clz(val[i-1]);
michaelrj retitled this revision from [libc] add division to UInt to [libc] add division, modulo, and power to UInt.Aug 31 2022, 11:29 AM
michaelrj edited the summary of this revision. (Show Details)
michaelrj marked 3 inline comments as done.

address comments

lntue accepted this revision.Aug 31 2022, 11:53 AM
michaelrj updated this revision to Diff 457334.Sep 1 2022, 11:20 AM

add bazel changes

This revision was landed with ongoing or failed builds.Sep 1 2022, 11:22 AM
This revision was automatically updated to reflect the committed changes.