[PATCH] D15120: Add support for __float128 type to be used by targets that support it
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Sun Dec 6 23:49:35 PST 2015
rsmith added a comment.
+rjmccall for `@encode` and USR mangling.
================
Comment at: include/clang-c/Index.h:2879-2885
@@ -2878,8 +2878,9 @@
CXType_LongDouble = 23,
- CXType_NullPtr = 24,
- CXType_Overload = 25,
- CXType_Dependent = 26,
- CXType_ObjCId = 27,
- CXType_ObjCClass = 28,
- CXType_ObjCSel = 29,
+ CXType_Float128 = 24,
+ CXType_NullPtr = 25,
+ CXType_Overload = 26,
+ CXType_Dependent = 27,
+ CXType_ObjCId = 28,
+ CXType_ObjCClass = 29,
+ CXType_ObjCSel = 30,
CXType_FirstBuiltin = CXType_Void,
----------------
Do not renumber these, the libclang ABI has stability guarantees. Add your new enumerator with value 30 instead.
================
Comment at: include/clang/Analysis/Analyses/ThreadSafetyTIL.h:244-245
@@ -243,2 +243,4 @@
}
+// FIXME: does this need a __float128 specialization as well?
+// Presumably not since most targets don't support it.
----------------
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.
================
Comment at: include/clang/Basic/TokenKinds.def:260
@@ -259,2 +259,3 @@
KEYWORD(double , KEYALL)
+KEYWORD(__float128 , KEYALL)
KEYWORD(else , KEYALL)
----------------
Please keep the list in alphabetical order, and in any case add this to the GNU extensions list below, not here.
================
Comment at: include/clang/Serialization/ASTBitCodes.h:813
@@ +812,3 @@
+ PREDEF_TYPE_OMP_ARRAY_SECTION = 55,
+ /// \brief The IEEE 754R 128-bit floating point type
+ PREDEF_TYPE_FLOAT128_ID = 56
----------------
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).
================
Comment at: lib/AST/ASTContext.cpp:972
@@ -971,2 +971,3 @@
TypeDecl *ASTContext::getFloat128StubType() const {
+ // FIXME: Is the assert (and indeed the function) needed?
assert(LangOpts.CPlusPlus && "should only be called for c++");
----------------
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`.
================
Comment at: lib/AST/ASTContext.cpp:8031
@@ -8015,2 +8030,3 @@
break;
+ // FIXME: we will probably need to handle __float128
case 'd':
----------------
We can defer that until we want to add a builtin that takes a `__float128`.
================
Comment at: lib/AST/ItaniumMangle.cpp:2061
@@ -2060,1 +2060,3 @@
break;
+ // FIXME: is this the right thing to do (always)?
+ case BuiltinType::Float128:
----------------
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++.)
================
Comment at: lib/AST/MicrosoftMangle.cpp:1708
@@ -1707,1 +1707,3 @@
+ // FIXME: We probably want to bail on this for now.
+ case BuiltinType::Float128:
----------------
Yes, that seems like the best thing to do for now; you can just drop this FIXME.
================
Comment at: lib/AST/NSAPI.cpp:444-445
@@ -443,2 +443,4 @@
case BuiltinType::UInt128:
+ // FIXME: Added for completeness. Not sure if it's needed.
+ case BuiltinType::Float128:
case BuiltinType::NullPtr:
----------------
This looks appropriate.
================
Comment at: lib/AST/StmtPrinter.cpp:1212-1213
@@ -1211,2 +1211,4 @@
case BuiltinType::LongDouble: OS << 'L'; break;
+ // FIXME: Need to figure out what this should be
+ case BuiltinType::Float128: OS << 'Q'; break;
}
----------------
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.
================
Comment at: lib/Basic/TargetInfo.cpp:225-228
@@ -223,2 +224,6 @@
case 128:
+ // FIXME: Return __float128 or long double on targets that support both?
+ // This placement of the condition will favour the former.
+ if (hasFloat128Type())
+ return Float128;
if (&getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble ||
----------------
Return `long double`. We should prefer the standard type over the GNU extension here.
================
Comment at: lib/Basic/Targets.cpp:1037-1044
@@ -1035,7 +1036,10 @@
bool useFloat128ManglingForLongDouble() const override {
return LongDoubleWidth == 128 &&
LongDoubleFormat == &llvm::APFloat::PPCDoubleDouble &&
getTriple().isOSBinFormatELF();
}
+ bool hasFloat128Type() const override {
+ return HasFloat128;
+ }
};
----------------
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.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1799
@@ -1798,2 +1798,3 @@
else {
+ // FIXME: does this need handling of __float128?
// Remaining types are either Half or LongDouble. Convert from float.
----------------
Yes. You should be able to test this by incrementing / decrementing a `__float128`.
================
Comment at: lib/Frontend/InitPreprocessor.cpp:717-718
@@ -716,2 +716,4 @@
DefineFloatMacros(Builder, "LDBL", &TI.getLongDoubleFormat(), "L");
+ // FIXME: we may need something like
+ //DefineFloatMacros(Builder, "FLT128", &TI.getFloat128Format(), "Q");
----------------
What macros does GCC define for `__float128`?
================
Comment at: lib/Sema/SemaLookup.cpp:665-672
@@ -664,9 +664,10 @@
if (II) {
if (S.getLangOpts().CPlusPlus11 && S.getLangOpts().GNUMode &&
II == S.getFloat128Identifier()) {
// libstdc++4.7's type_traits expects type __float128 to exist, so
// insert a dummy type to make that header build in gnu++11 mode.
+ // FIXME: Is this still needed since __float128 becomes a built-in type?
R.addDecl(S.getASTContext().getFloat128StubType());
return true;
}
if (S.getLangOpts().CPlusPlus && NameKind == Sema::LookupOrdinaryName &&
----------------
Delete this entire `if` block.
================
Comment at: lib/Sema/SemaOverload.cpp:1907-1908
@@ -1906,4 +1906,4 @@
// C99 6.3.1.5p1:
// When a float is promoted to double or long double, or a
// double is promoted to long double [...].
if (!getLangOpts().CPlusPlus &&
----------------
This should also allow promoting `long double` to `__float128`.
================
Comment at: lib/Sema/SemaType.cpp:6696-6702
@@ -6688,8 +6695,9 @@
+ // FIXME: Is this check still needed since __float128 becomes a built-in type?
// We have an incomplete type. Produce a diagnostic.
if (Ident___float128 &&
T == Context.getTypeDeclType(Context.getFloat128StubType())) {
Diag(Loc, diag::err_typecheck_decl_incomplete_type___float128);
return true;
}
----------------
This isn't necessary any more; remove it.
Repository:
rL LLVM
http://reviews.llvm.org/D15120
More information about the cfe-commits
mailing list