This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add explicit constructor calls to fix compilation when using UInt<T>
ClosedPublic

Authored by mikhail.ramalho on May 1 2023, 9:22 AM.

Details

Summary

This patch is similar to 86fe88c8d9 and adds several explicit
constructor calls (bool(...), uint64_t(...), uint8_t(...)) that are
needed when we use UInt<T> (in my case UInt<128> in riscv32).

This patch also adds two operators to UInt<T>:

  • operator/= required by printf_core/float_hex_converter.h:148
  • operator-- required by FPUtil/ManipulationFunctions.h:166

Diff Detail

Event Timeline

mikhail.ramalho created this revision.May 1 2023, 9:22 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 1 2023, 9:22 AM
mikhail.ramalho requested review of this revision.May 1 2023, 9:22 AM
sivachandra accepted this revision.May 1 2023, 10:00 AM
sivachandra added a subscriber: lntue.

OK from my side after addressing the nits but please wait for @lntue and @michaelrj also.

libc/src/__support/FPUtil/generic/sqrt.h
139

Nit: We are slowly switching over to use static_cast over bool(...) which apparently is equivalent to C like casts.

libc/src/__support/float_to_string.h
206

Same here and other places in this patch.

This revision is now accepted and ready to land.May 1 2023, 10:00 AM
michaelrj added inline comments.May 1 2023, 10:32 AM
libc/src/__support/UInt.h
748

This should probably not be constexpr, and neither should operator++ since they call add and sub which use the non-const versions of add_with_carry and sub_with_borrow. You can add a constexpr version of those functions similar to how operator+ and operator- are if you want.

mikhail.ramalho marked 3 inline comments as done.
  • Use static_cast<foo> instead of foo(...)
  • Changed operator++ and operator-- to use operator+ and operator-, respectively
libc/src/__support/FPUtil/generic/sqrt.h
139

tbh, I thought this was calling the bool constructor. I'll change it.

lntue accepted this revision.May 1 2023, 12:01 PM
mikhail.ramalho reopened this revision.May 19 2023, 2:38 PM
This revision is now accepted and ready to land.May 19 2023, 2:38 PM
  • Implement get_val() for x86_64 long double class
  • Use dyadic float in clinger_fast_path to fix conversion to long double
lntue added inline comments.May 19 2023, 5:24 PM
libc/src/__support/UInt.h
83

Bits >= 128

486

Should the return type be UInt<Bits>&?

sivachandra added inline comments.May 20 2023, 10:03 PM
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
131

Why was this method added? In other words, why was not required previously?

libc/src/__support/str_to_float.h
311

Why not this directly:

exp2 -= static_cast<uint32_t>(1) ^ static_cast<uint32_t>(msb);
libc/src/stdio/printf_core/float_dec_converter.h
1154

FPBits<double>::UIntType

1175

Ditto

1196

Ditto

libc/src/stdio/printf_core/float_hex_converter.h
55

Ditto

libc/src/stdio/printf_core/float_inf_nan_converter.h
40

Ditto

libc/src/stdio/printf_core/int_converter.h
46

Why not uintmax_t directly?

mikhail.ramalho marked 8 inline comments as done.

Addressed comments

libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
131

That's because of the new dyadic_float object used in clinger_fast_path.

DyadicFloat's conversion operator calls get_val(), but this was only implemented in the non-x86_64 class.

libc/src/__support/UInt.h
486

I followed how it's done for the other operators, do you want me to change just this one?

michaelrj added inline comments.May 22 2023, 2:32 PM
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
129

I'm not sure this is correct, this seems to be setting the explicit bit in the output regardless of if it is set in the actual number. An example of where this might go wrong is the number 0.0.

mikhail.ramalho marked an inline comment as done.
  • Rebased
  • Fix zero conversion
  • Added test case for 0
lntue added inline comments.Jun 6 2023, 10:32 AM
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
128

is_zero() is also true for -0.0L. You'll need to check for sign:

return get_sign() ? -0.0L : 0.0L;
libc/src/__support/UInt.h
486

Please fix other op= signature to return references if you see them. Thanks!

michaelrj added inline comments.Jun 6 2023, 1:12 PM
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
129

I meant more that if bits is accurate, then you shouldn't need to add the explicit bit separately at all. In get_explicit_mantissa it expands the mask to cover the explicit leading bit, but doesn't actually modify the resulting number at all.

This patch no longer needed.