Page MenuHomePhabricator

Revert "Teach __libcpp_is_floating_point that __fp16 and _Float16 are"
ClosedPublic

Authored by phosek on Oct 24 2018, 2:01 PM.

Details

Summary

This reverts commits r333103 and r333108. _Float16 and __fp16 are C11
extensions and compilers other than Clang don't define these for C++.

See D53486 for related discussion.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Oct 24 2018, 2:01 PM
ldionne accepted this revision.Oct 24 2018, 2:25 PM
This revision is now accepted and ready to land.Oct 24 2018, 2:25 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

Sorry I'm late to the party, but we should just be falling back to __is_floating_point if we have it. Since it will always give us the answer we want. (Including for __float128 which we don't handle ATM).

Sorry I'm late to the party, but we should just be falling back to __is_floating_point if we have it. Since it will always give us the answer we want. (Including for __float128 which we don't handle ATM).

That doesn't change the fact that it would be lying about what it means to be a floating point type, wouldn't it? For example, numeric_limits would still not be defined for __fp16, right?

As a side question, do you know how numeric_limits are implemented for __float128?

Sorry I'm late to the party, but we should just be falling back to __is_floating_point if we have it. Since it will always give us the answer we want. (Including for __float128 which we don't handle ATM).

That doesn't change the fact that it would be lying about what it means to be a floating point type, wouldn't it? For example, numeric_limits would still not be defined for __fp16, right?

If it acts like a floating point type, and the compiler says it's a floating point type, then it's a floating point type; regardless of what the C++ standard says.
We shouldn't pretend these types don't exist.

If the compiler provides the types, it means someone went through a ton of trouble in order to provide these additional types to users. So the question we should be asking is "What's in the best interest of our users?",

As a side question, do you know how numeric_limits are implemented for __float128?

rsmith added a subscriber: rsmith.Oct 24 2018, 4:13 PM

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

There's also the fact that e.g. __fp16 doesn't work with the stream's operator>> and similar problems. Because of this, we're effectively lying about the fact that __fp16 is a floating point type. This was Marshall's point, and I agree with it.

@EricWF I do agree that if it quacks like a floating point and it walks like a floating point, then it is a floating point. However, __fp16 doesn't seem to quack quite right, so it's not a floating point (at least not without additional work).

Sorry I'm late to the party, but we should just be falling back to __is_floating_point if we have it. Since it will always give us the answer we want. (Including for __float128 which we don't handle ATM).

@EricWF and I discussed this privately, and I disagree.
There's lots of things in the standard library that key off of std::is_floating_point, and we don't support *any* of them for __fp16 or _Float16 or __float128.

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

There's also the fact that e.g. __fp16 doesn't work with the stream's operator>> and similar problems. Because of this, we're effectively lying about the fact that __fp16 is a floating point type. This was Marshall's point, and I agree with it.

@EricWF I do agree that if it quacks like a floating point and it walks like a floating point, then it is a floating point. However, __fp16 doesn't seem to quack quite right, so it's not a floating point (at least not without additional work).

I do mostly agree with this for __fp16 (which is a weird storage-only type with some arbitrary limitations). But not for _Float16 nor __float128, which are both proper floating-point types.

For whatever it's worth, in libstdc++ std::is_floating_point<__float128>::value yields true by default (which in this case means in -std=gnu++XY but not in -std=c++XY) on targets that have a __float128 type. They also supply an abs overload for __float128 in that case, but nothing else (surprisingly not even numeric_limits).

For whatever it's worth, in libstdc++ std::is_floating_point<__float128>::value yields true by default (which in this case means in -std=gnu++XY but not in -std=c++XY) on targets that have a __float128 type. They also supply an abs overload for __float128 in that case, but nothing else (surprisingly not even numeric_limits).

Yes, but clang doesn't have a "not a standard" mode like gcc does. (nor do I think we should have one).

In my opinion, the proper way forward is to standardize a notion of "extended floating point types" and establish a set of things that those must support. Then, when vendors want to provide such types, they can make sure that we (the library implementation) can implement the required functionality for their custom types to be proper floating point types. This would mean abs , isnan, numeric_limits, etc...

We don't have such a notion right now. If we did, we could arguably make _Float16 and __float128 such extended floating point types, but in all cases __fp16 would be dead in the water. I hear that someone is going to propose such floating point types -- I would be inclined to wait and see how this pans out before we spend more cycles trying to figure the issue out ourselves.