This is an archive of the discontinued LLVM Phabricator instance.

Add support for __float128 type to be used by targets that support it
ClosedPublic

Authored by nemanjai on Dec 1 2015, 10:05 AM.

Details

Summary

This patch adds support for the __float128 keyword and literals for IEEE Quad Precision.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 41524.Dec 1 2015, 10:05 AM
nemanjai retitled this revision from to Add support for __float128 type to be used by targets that support it.
nemanjai updated this object.
nemanjai added reviewers: hfinkel, wschmidt, kbarton, rsmith.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: cfe-commits.

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.

rsmith edited edge metadata.

+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.

rjmccall added inline comments.Dec 7 2015, 12:16 AM
lib/AST/ASTContext.cpp
5456

Please add this as a case to the block below for Half. I'll ping internally for a decision on what @encoding to use.

lib/Index/USRGeneration.cpp
604

I'm pretty sure you can just choose something here that doesn't collide, but the right person to ask is Argyrios.

nemanjai updated this revision to Diff 42414.Dec 10 2015, 4:54 AM
nemanjai edited edge metadata.

Updated to address review comments. Rather than listing responses and justification for some of the changes, I'll provide comments inline for better readability.

nemanjai added inline comments.Dec 10 2015, 5:19 AM
lib/AST/ItaniumMangle.cpp
2066

Please refer to https://gcc.gnu.org/ml/gcc-patches/2015-10/txt5mF4d1XnFb.txt.
GCC recently added this for PowerPC and the new mangling was provided for the __float128 type when it coexists with long double.

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:

[...] its value is unchanged.

Allowing long double to promote to __float128 violates that on at least one target platform.
For example, PPCDoubleDouble can represent (2^512) - 1 exactly.

nemanjai added inline comments.Dec 11 2015, 5:15 AM
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?

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.

nemanjai updated this revision to Diff 44459.Jan 11 2016, 3:38 AM
nemanjai updated this object.

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.

rjmccall added inline comments.Jan 13 2016, 9:57 PM
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).

nemanjai updated this revision to Diff 45998.Jan 26 2016, 10:22 AM

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.

rjmccall edited edge metadata.Jan 26 2016, 12:02 PM

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.

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.

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.

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.

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.

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.

I believe that decimal floating point types introduced the situation of having two arithmetic types that don't have a common type into "C".

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 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.

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.

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.

The applicable wording is quoted here: D15120#310972, but I guess that's unimportant for the purposes this discussion now.

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?

This comment was removed by hubert.reinterpretcast.

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.

This comment was removed by hubert.reinterpretcast.

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.

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.

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.

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).

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.

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).

Do we have anyone involved in this standardization effort? It seems like a really poor idea to having type ranking be target-dependent.

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.

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).

Do we have anyone involved in this standardization effort? It seems like a really poor idea to having type ranking be target-dependent.

I can reach out to someone who is involved.

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.

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?

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:

  1. Remove the diagnostics
  2. Allow the conversions
  3. Provide libcalls for the necessary operations (similarly to what GCC does)

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:

  1. Remove the diagnostics
  2. Allow the conversions
  3. Provide libcalls for the necessary operations (similarly to what GCC does)

This sounds good to me. @rjmccall @rsmith, are we good to move forward with this first patch?

lib/Sema/SemaExpr.cpp
1156

The result of the dyn_cast can be saved instead of using cast after the dyn_cast.

rsmith added inline comments.Feb 3 2016, 3:16 PM
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)?

nemanjai updated this revision to Diff 47223.Feb 8 2016, 10:27 AM
nemanjai edited edge metadata.

Fixed the naming, indentation and removed the use of dyn_cast for types.

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)

nemanjai added inline comments.Feb 10 2016, 1:06 AM
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:

  • One type is __float128
  • The other is long double
  • The representation of long double is not llvm::APFloat::IEEEdouble

Basically in this function, the early exit path would be:

  • representations are the same or
  • representation of long double is llvm::APFloat::IEEEdouble
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.

nemanjai updated this revision to Diff 49508.Mar 1 2016, 10:48 AM

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

The update to check the case where the representation of long double is the same as that of double is missing.

1963–1964

@rsmith; this is the discussion on floating-point "promotions" in "C" that I was asking you about.

nemanjai added inline comments.Mar 2 2016, 4:51 AM
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).

rsmith accepted this revision.Mar 3 2016, 2:50 PM
rsmith edited edge metadata.

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?

This revision is now accepted and ready to land.Mar 3 2016, 2:50 PM
nemanjai closed this revision.Apr 13 2016, 8:14 AM

Committed revision 266186.