This patch adds support for the __float128 keyword and literals for IEEE Quad Precision.
Diff Detail
- Repository
- rL LLVM
Event Timeline
Just a friendly reminder to the reviewers that this patch has been up for almost a week with no review comments. Please take some time to review.
+rjmccall for @encode and USR mangling.
include/clang-c/Index.h | ||
---|---|---|
2912–2914 | Do not renumber these, the libclang ABI has stability guarantees. Add your new enumerator with value 30 instead. | |
include/clang/Analysis/Analyses/ThreadSafetyTIL.h | ||
244–245 ↗ | (On Diff #41524) | I don't know, and this template looks suspicious already. We likely shouldn't be assuming that long double's size is 128 in the previous function. |
include/clang/Basic/TokenKinds.def | ||
260 | Please keep the list in alphabetical order, and in any case add this to the GNU extensions list below, not here. | |
include/clang/Serialization/ASTBitCodes.h | ||
813 | I would say "The '__float128' type." here -- we don't need to assume that it's *this* 128-bit floating point type on any particular platform (except in the setup for that particular target). | |
lib/AST/ASTContext.cpp | ||
969 | This function only existed to allow us to parse libstdc++'s numeric_limits specialization for __float128. We won't need it any more once we have real support for __float128. | |
8085 | We can defer that until we want to add a builtin that takes a __float128. | |
lib/AST/ItaniumMangle.cpp | ||
2064 | We should test a platform that uses useFloat128ManglingForLongDouble. Presumably they must either not support __float128 or use a different mangling here. (IIRC, they can't make long double and __float128 the same type because that would break libstdc++.) | |
lib/AST/MicrosoftMangle.cpp | ||
1764 | Yes, that seems like the best thing to do for now; you can just drop this FIXME. | |
lib/AST/NSAPI.cpp | ||
444–445 | This looks appropriate. | |
lib/AST/StmtPrinter.cpp | ||
1279–1280 | Does GCC support a suffix for __int128? If not, just add a FIXME like we do for Half. This case -- presumably -- should never happen if there are no literals of this type. | |
lib/Basic/TargetInfo.cpp | ||
226–229 | Return long double. We should prefer the standard type over the GNU extension here. | |
lib/Basic/Targets.cpp | ||
1054–1062 | Should we really support both __float128 and using the __float128 mangling for long double simultaneously? That seems likely to be broken unless there's another mangling for __float128 that we can use. | |
lib/CodeGen/CGExprScalar.cpp | ||
1797 | Yes. You should be able to test this by incrementing / decrementing a __float128. | |
lib/Frontend/InitPreprocessor.cpp | ||
717–718 | What macros does GCC define for __float128? | |
lib/Sema/SemaLookup.cpp | ||
671–672 | Delete this entire if block. | |
lib/Sema/SemaOverload.cpp | ||
1963–1964 | This should also allow promoting long double to __float128. | |
lib/Sema/SemaType.cpp | ||
6758–6764 | This isn't necessary any more; remove it. |
Updated to address review comments. Rather than listing responses and justification for some of the changes, I'll provide comments inline for better readability.
lib/AST/ItaniumMangle.cpp | ||
---|---|---|
2066 | Please refer to https://gcc.gnu.org/ml/gcc-patches/2015-10/txt5mF4d1XnFb.txt. | |
lib/AST/StmtPrinter.cpp | ||
1279 | As in GCC, __float128 literals in source code have a suffix 'q'/'Q'. I'll be perfectly honest - I don't know when this function is called, but it seems that for consistency, we should use the same suffix here. | |
lib/Basic/TargetInfo.cpp | ||
229 | Left this condition in but moved it down. This way if a target exists that has 'long double' that is narrower than 128 bits, but it supports __float128, we can still return a 128-bit floating point type here. | |
lib/Basic/Targets.cpp | ||
1060 | Now that we have different mangling for float128 and long double, it should be safe to support float128 and leave the mangling of long double unchanged. | |
1253 | GCC defines this macro when -mfloat128 is specified. | |
lib/CodeGen/CGExprScalar.cpp | ||
1797 | An expression incrementing a variable of __float128 type was added to the test case. | |
lib/Frontend/InitPreprocessor.cpp | ||
718 | GCC does not do this at this particular time, but it seems logical to actually provide this macro on targets that support it. For PPC, the macros are defined (and tested) to match IEEE quad precision values (when -mfloat128 is specified). | |
lib/Index/USRGeneration.cpp | ||
603 | As far as I can tell, this does not collide and it is consistent with the literal suffix for this type. However, I'd be happy to change it if there is a better suffix to use. Do you have a different suggestion @akyrtzi? |
I would like to see a test:
__float128 qf(); long double ldf(); long double ld{qf()}; // error: narrowing __float128 q{ldf()}; // passes
include/clang/Basic/TargetInfo.h | ||
---|---|---|
384 | This should probably not be hard-coded. On s390x-ibm-linux-gnu, the IEEE 128-bit binary format type has 8 byte alignment (not 16). | |
lib/CodeGen/CGDebugInfo.cpp | ||
548 | Is a PPCDoubleDouble "long double" and "__float128" distinguishable on PPC given this DWARF treatment? | |
lib/Lex/LiteralSupport.cpp | ||
604 | minor: should the comment have FL instead of LF? | |
lib/Sema/SemaOverload.cpp | ||
1963–1964 | In C99 6.3.1.5p1:
Allowing long double to promote to __float128 violates that on at least one target platform. |
include/clang/Basic/TargetInfo.h | ||
---|---|---|
384 | OK, I probably shouldn't have assumed anything about the alignment. I'll add fields to the target info that can be set as needed. | |
lib/CodeGen/CGDebugInfo.cpp | ||
548 | Unless there's something put into the debugger to figure out which type something actually is, I don't think so. However, there isn't a consensus on what encoding will be used, so I'll just leave it as-is and put a FIXME in the code to note this. | |
lib/Lex/LiteralSupport.cpp | ||
604 | I agree that it probably should, but it was already there so I didn't change it. I'll change it on the next review. |
I would also like to see a test case based on the following from ISO/IEC TS 18661-3:2015:
If both operands have floating types and neither of the sets of values of their corresponding real types is a subset of (or equivalent to) the other, the behavior is undefined.
auto f(__float128 q, long double ld) { return q + ld; // error: no common type }
I think the correct course of action would be to allow/disallow promotion based on a condition that the two types are the same/different (respectively). I think a comparison of the float semantics is a valid way to check this. Also, should operations between long double and __float128 be diagnosed as warnings or errors if the two types are distinct?
When the "two" types have the same float semantics, an additional consideration is whether there are indeed two types, or if there is just one (with different ways to name it). On x86_64 Linux systems, I believe that the latter case is true for __float80 and long double with GCC; however, with -mlong-double-128, GCC treats __float128 and long double as distinct (but uses the same mangling for both). That is to say, GCC is inconsistent.
With regards to the usual arithmetic conversions, I believe that an error is the safest course of action when the two types are distinct (between PPCDoubleDouble and IEEEquad). It appears that the use of IsFloatingPointPromotion for C is sketchy (see inline comments).
lib/Sema/SemaOverload.cpp | ||
---|---|---|
1963–1964 | The only "promotions" in C as of C11 are the integer promotions and the default argument promotions. The (now removed) use of "promoted" to describe a conversion between double to long double appears to have been an unintentional overloading of the term. If the purpose of this function is as described (to implement conv.fpprom), then the use of it for C appears to be erroneous. At the very least, the comments are not sufficient and the naming of the function (given its current implementation) no longer has a basis in the C Standard. |
Addressed the review comments:
- Added the requested test cases
- Disabled promotion of long double to __float128
- Disabled operations between long double and __float128 if they have different representations
lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
550 | There are various typos in this comment. | |
lib/CodeGen/CGExprScalar.cpp | ||
1806 | It might make sense to move the call to getLongDoubleFormat() to the if/else block and perhaps to order it as long double/__float128/half. | |
lib/Sema/SemaExpr.cpp | ||
1115 | Are we all comfortable that if long double and __float128 have the same representation, but are considered distinct types, then the LHS type is used? Is this consistent with GCC? | |
1146 | There's no actual need for the .getTypePtr() part; the overloaded operator-> does the same thing. | |
1153 | LHSComplexType is only used here. It could be replaced with cast<ComplexType>(LHSType.getTypePtr()) (and similarly for the RHS). Indeed, since we are checking against specific real element types anyway, dyn_cast<ComplexType>(LHSType.getTypePtr()) (and checking for null) would suffice (no need to use isComplexType()). | |
test/Sema/float128-ld-incompatibility.cpp | ||
33 | I believe a test for the conditional operator would be appropriate. |
include/clang/Basic/TargetInfo.h | ||
---|---|---|
384 | I think you can safely have this method hard-code the size, though. :) | |
lib/AST/ASTContext.cpp | ||
4628 | Follow the same code styling as above, please. | |
4644 | Indentation. | |
4668 | This is a really bad language rule with nasty implications for a lot of code. Is there a really urgent need to emulate some other compiler bug-for-bug here? It's okay for there to a formal rank difference even when types have the same representation — that's basically always true for the integer types — so unless there's a very specific and very good reason not to, let's just use the rule that float128 has higher rank than long double. |
lib/AST/ASTContext.cpp | ||
---|---|---|
4668 | I am not very keen on the implications of having the equal rank either. I believe the case where neither type represents the full set of values of the other should be considered "unordered" though (currently that is handled by typesNotCompatible(), which is in need of a better name since the function does not deal with "compatible types" as defined by C11 subclause 6.2.7). |
Addressed review comments.
The key differences are:
- No assignments or operations between entities of long double and __float128 allowed if the two types have a different representation
- Each type has a distinct rank
This isn't the same behaviour as GCC currently has. For example, GCC allows assignments between the two types on PowerPC. The conversion is achieved through a library routine. However, there is no compelling reason at this point to allow such behaviour and this work is deferred until such a need arises.
As I understand it, PPC's long-double (~103 bits of precision) is still strictly less precise than float128_t (113 bits of precision), so it ought to be have lower rank. Is there actually a supported platform where this is not true? If not, we should just add this as another type with higher rank.
PPC's long-double has variable precision. It is not strictly less precise than float128_t.
Ah, right. I was thinking of it as if the lower double were pegged to 1/2 an ulp of the higher double, but it isn't; it can be variably lower.
Here's the thing, though: I don't think there's a reasonable language solution here besides saying that float128_t has higher rank. You can't make the types incompatible, because it's clearly reasonable to simply convert one to the other. What you're trying to say is that they don't have a common type, but it's a novel concept in C/C++ to have two arithmetic types that don't have a common type and therefore cannot be added / compared / ternary'd together. In contrast, it is not a novel concept to have a type that implicitly promotes to another type but potentially loses precision, because all the integer types will happily convert to float/double.
This patch will be much simpler, and you will get a better language design, if you simply make float128_t a new FP type with a higher rank than long double.
I believe that decimal floating point types introduced the situation of having two arithmetic types that don't have a common type into "C".
This patch will be much simpler, and you will get a better language design, if you simply make float128_t a new FP type with a higher rank than long double.
The C committee decided that "undefined behavior" was what they could agree on for this sort of case. I suppose that on most platforms float128_t logically has the higher rank and it would be a shame to have source portability issues because the expression would need more casts on PPC. Even the narrowing conversion semantics in C++ is fine with losing precision.
Perhaps; I don't know those rules at all.
This patch will be much simpler, and you will get a better language design, if you simply make float128_t a new FP type with a higher rank than long double.
The C committee decided that "undefined behavior" was what they could agree on for this sort of case.
That's only when the operand value is actually outside of the range of the type, which for implementations claiming IEEE 754 compatibility means "never" because of the infinities. Even if it weren't specified, every implementation I know of in practice does give this defined behavior using the active FP rounding rules.
Anyway, I think we're agreed that we should just give make float128_t higher rank on all platforms.
The applicable wording is quoted here: D15120#310972, but I guess that's unimportant for the purposes this discussion now.
Anyway, I think we're agreed that we should just give make float128_t higher rank on all platforms.
Agreed.
Oh, I see, you were referring to an FP->FP conversion rule, whereas I was still talking about the int->FP conversion rule as an example where C accepts converting to a common FP type even when it loses precision.
That TS rule is extremely strange; I wonder whether they really mean "undefined", or if they just mean "implementation-defined", or if they just meant to say that it's UB if the original value isn't a member of the result type. I really don't know why you would intentionally give an entire statically-determined operation dynamically undefined behavior instead of simply making it ill-formed. Sometimes TSes aren't drafted very carefully.
Oh well, a question for another day.
Thank you for the discussion on rank. I'm glad we have an agreement on the rank of the two types.
However, considering __float128 already has a higher rank in this patch, is there anything else that you would like me to change here before the patch is approved?
Do you suggest that we need to allow operations (or at least assignments) between the two types and take away the diagnostics that are part of this patch?
My overriding concern at this time is to allow (or not) the operations consistently (that is, not making it platform-specific). The net result is that we will allow operations between the two types; however, we will encounter a further issue to discuss (@rjmccall @rsmith; your input requested) regarding what the common type should be when the representations are the same (the TS prefers the standard floating type, which is long double here).
As for the diagnostics, the IEEEquad to PPCDoubleDouble conversion still requires checks for narrowing.
I think it's not unlikely that float128_t will see future standardization (as an optional extension, of course), and it would be strange for an operation between two types to not consistently yield the type of higher rank.
I could see an argument that we should not treat float128_t as a distinct type from long double on targets where they have the same implementation. That might avoid the need for special-case manglings.
It remains that the present standardization effort (as _Float128) does not imbue the "interchange" type with inherently higher rank than long double. In a parallel to the treatment of extended integer types, the "standard" type has higher rank when the set of values are equivalent between the two. This is consistent with the GCC implementation (online compiler: http://melpon.org/wandbox/permlink/tM3PyGWC5WTWIdKP).
I could see an argument that we should not treat float128_t as a distinct type from long double on targets where they have the same implementation. That might avoid the need for special-case manglings.
I would prefer this as well (insofar as it saves us from the common type issue). As I have mentioned before, for __float128 and -mlong-double-128 on x86-64, GCC ends up with an undesirable situation of treating the types as distinct, but mangling them the same. In the TS, _Float128 is always distinct from long double, which is helpful for portability of _Generic expressions with both types. In the end, it seems to come down to what sort of code people will write. If overloads for both __float128 and long double are the norm, then we will need to consider the types distinct.
Do we have anyone involved in this standardization effort? It seems like a really poor idea to having type ranking be target-dependent.
I could see an argument that we should not treat float128_t as a distinct type from long double on targets where they have the same implementation. That might avoid the need for special-case manglings.
I would prefer this as well (insofar as it saves us from the common type issue). As I have mentioned before, for __float128 and -mlong-double-128 on x86-64, GCC ends up with an undesirable situation of treating the types as distinct, but mangling them the same.
Does Clang currently support that option?
In the TS, _Float128 is always distinct from long double, which is helpful for portability of _Generic expressions with both types. In the end, it seems to come down to what sort of code people will write. If overloads for both __float128 and long double are the norm, then we will need to consider the types distinct.
Makes sense.
I can reach out to someone who is involved.
As I have mentioned before, for __float128 and -mlong-double-128 on x86-64, GCC ends up with an undesirable situation of treating the types as distinct, but mangling them the same.
Does Clang currently support that option?
It appears that Clang does not currently support that option; however, there are platforms where long double is already IEEE quad, e.g., s390x-ibm-linux-gnu (where I have not found a GCC providing __float128). It appears that we can defer the issue as long as we do not provide __float128 when long double is already IEEE quad.
Thank you. If they have a strong motivation here, that's fine, but this feels like it would block and complicate future standardization efforts, as well as impair portability.
As I have mentioned before, for __float128 and -mlong-double-128 on x86-64, GCC ends up with an undesirable situation of treating the types as distinct, but mangling them the same.
Does Clang currently support that option?
It appears that Clang does not currently support that option
Ok. It would be better if we can avoid that, I think.
; however, there are platforms where long double is already IEEE quad, e.g., s390x-ibm-linux-gnu (where I have not found a GCC providing __float128).
Sure, that's a legitimate platform ABI choice.
It appears that we can defer the issue as long as we do not provide __float128 when long double is already IEEE quad.
Sounds good.
It appears that the conclusion is that operations between the two types will be allowed. The common type between __float128 and long double will be __float128 at this time, based on the observation that __float128 is not yet available in cases where the choice of common type is still unclear.
It appears that at some point, interoperability issues with GCC will change that, and the choice of common type may need to be a target-based property (which may be implemented by the rank being target-based). For the purposes of this patch, I think we are deferring that issue.
If the reviewers don't mind, I would like to keep this patch with diagnostics for interoperability between the two types for now. This is simply because enabling such interoperability requires changes to some of the conversion infrastructure (i.e. allowing FPTrunc/FPExt for types of the same width, etc.). This is to prevent crashes on code such as:
__float128 foo(long double d) { return d; }
A test case like that will trip asserts when attempting to generate code. Of course, this is easy to fix (3 minor changes in 2 files) but even if we emit that IR, the back end will fail when trying to compile it.
What I meant to do with this patch is to just get the Clang support in and emit diagnostics for things that the target isn't able to do yet. I will follow this up with a patch that will:
- Remove the diagnostics
- Allow the conversions
- Provide libcalls for the necessary operations (similarly to what GCC does)
include/clang/Driver/Options.td | ||
---|---|---|
1463 | Per the mangling rules at the top of this file, this should be named mno_float128. | |
lib/CodeGen/ItaniumCXXABI.cpp | ||
3357–3358 | Please keep this in the same order as TypeInfoIsInStandardLibrary (put Float128Ty after LongDoubleTy). | |
lib/Sema/SemaExpr.cpp | ||
1156–1159 | Do not use dyn_cast / cast on types, use LHSType->getAs<...>() / LHSType->castAs<...>() instead. This will do the wrong thing on sugared types (eg, a typedef for a complex type). | |
1337 | Please indicate why you're diagnosing these here. | |
1339 | Too much indentation. | |
lib/Sema/SemaOverload.cpp | ||
1655–1664 | This is over-indented. |
lib/Sema/SemaExpr.cpp | ||
---|---|---|
1156–1159 | Are there contexts in Clang where sugared types don't occur? Is the general guidance to use getAs or castAs even if the types are known to be canonical (which I agree isn't the case here)? |
lib/Basic/TargetInfo.cpp | ||
---|---|---|
230 | Is it necessary to check that __float128 is IEEE quad here? Unlike the long double cases, __float128 really better be 128 bits. | |
lib/Sema/SemaExpr.cpp | ||
1169 | I do not believe that the width of long double is sufficiently established to be the same as that of __float128 in this context. | |
1338 | s/such type/such types; | |
lib/Sema/SemaOverload.cpp | ||
1655 | Is conversion between long double and __float128 the correct characterization of the missing back-end support? (as opposed to conversion between PPCDoubleDouble and IEEEquad) |
lib/Basic/TargetInfo.cpp | ||
---|---|---|
230 | It would indeed be weird if it wasn't so. I can remove the second condition in that if. | |
lib/Sema/SemaExpr.cpp | ||
1169 | This would certainly cause any attempts to convert between some different implementation of long double (i.e. the Intel 80-bit type). I believe that this behaviour is desired in all cases where long double and double have a different representation. Namely, we do not currently have any support for converting between fp128 and anything that has precision >= double that is not actually double. What I propose to do here and in other locations where we diagnose conversions between the two types is that the check is:
Basically in this function, the early exit path would be:
| |
1338 | Oops, I dropped the s. I'll fix it. | |
lib/Sema/SemaOverload.cpp | ||
1655 | Well, the same issue exists with x86_fp80. We don't have libcalls set up for handling that either. Of course the intended semantics and the comment are still not quite correct here. Really, what I think we should be after is catching attempts to convert between __float128 and long double on targets where long double is not actually just double. |
lib/Sema/SemaExpr.cpp | ||
---|---|---|
1169 | This really sounds like a property of the target. The situation described in D15120#inline-141210 is similar. I would not want to do a virtual function call without early exits here. So, perhaps the TargetInfo should be queried only at the point where we would otherwise return true. I can't say that I like the name of this function now. Perhaps unsupportedTypeConversion? |
lib/Frontend/InitPreprocessor.cpp | ||
---|---|---|
718 | I am concerned that setting these macros are premature. We still do not know if the _Float128 type from the C Technical Specification (which has claim to FLT128_*, although not necessarily the double-underscore versions) will be the same type as __float128. The latter may be considered typedef-equivalent to long double; the former shall not be. |
Removed questionable macro definitions.
Renamed the test function for invalid conversions and changed the semantics so that it allows float128 <-> long double conversions only if the two types have the same representation or the latter has the same representation as double.
Removed unnecessary check for the representation of float128 when returning a float type of width 128.
lib/Sema/SemaOverload.cpp | ||
---|---|---|
1655 | Yes, I missed this. Thanks for noticing. I'll update the comment and change the body of the if to: if (&S.Context.getFloatTypeSemantics(FromType) != &S.Context.getFloatTypeSemantics(ToType)) { bool Float128AndLongDouble = ((FromType == S.Context.Float128Ty && ToType == S.Context.LongDoubleTy) || (FromType == S.Context.LongDoubleTy && ToType == S.Context.Float128Ty)); if (Float128AndLongDouble && (&S.Context.getFloatTypeSemantics(S.Context.LongDoubleTy) != &llvm::APFloat::IEEEdouble)) return false; } | |
1963–1964 | I can certainly remove this if the agreement is that I should do so. |
lib/Sema/SemaOverload.cpp | ||
---|---|---|
1655 | I'm not a fan of redundant parentheses, but that looks okay. | |
1963–1964 | Richard had requested to allow "promoting" long double to __float128, and at the time, implicitly converting from long double to __float128 was something I was not convinced we should do at the time. At this time, I believe that adding this "promotion" is consistent with the rest of the patch; however, I am still not sure what this code is meant to implement (again, C11 does not have floating-point promotions). |
LGTM
lib/Sema/SemaOverload.cpp | ||
---|---|---|
1963–1964 | I think our justification for allowing this promotion in C doesn't make any sense. The C wording is just trying to give slightly different rules for the "type gets bigger" and "type gets smaller" case and "promotion" here is an unfortunate choice of words. C99 didn't have floating point promotions. I'm inclined to think we should remove this special case entirely. Does that cause any test failures? |
Do not renumber these, the libclang ABI has stability guarantees. Add your new enumerator with value 30 instead.