Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Sema/SemaOverload.cpp:1658 @@ -1657,1 +1657,3 @@ } else if (FromType->isRealFloatingType() && ToType->isRealFloatingType()) { +// FIXME: disable conversions between long double and __float128 if +// their

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Sema/SemaOverload.cpp:1658 @@ -1657,1 +1657,3 @@ } else if (FromType->isRealFloatingType() && ToType->isRealFloatingType()) { +// FIXME: disable conversions between long double and __float128 if +// their

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai updated this revision to Diff 49508. nemanjai added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Frontend/InitPreprocessor.cpp:718 @@ -717,1 +717,3 @@ + if (TI.hasFloat128Type()) +DefineFloatMacros(Builder, "FLT128", (), "Q"); I am concerned that setting these macros are premature. We still

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Sema/SemaExpr.cpp:1169 @@ +1168,3 @@ + if (LHSElemType == S.Context.Float128Ty && + RHSElemType == S.Context.LongDoubleTy) +return true; nemanjai wrote: > hubert.reinterpretcast wrote: > > I

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added inline comments. Comment at: lib/Basic/TargetInfo.cpp:231 @@ +230,3 @@ +if (hasFloat128Type() && +() == ::APFloat::IEEEquad) + return Float128; hubert.reinterpretcast wrote: > Is it necessary to check that `__float128` is IEEE quad

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Basic/TargetInfo.cpp:231 @@ +230,3 @@ +if (hasFloat128Type() && +() == ::APFloat::IEEEquad) + return Float128; Is it necessary to check that `__float128` is IEEE quad here? Unlike the

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai updated this revision to Diff 47223. nemanjai added a comment. Fixed the naming, indentation and removed the use of dyn_cast for types. Repository: rL LLVM http://reviews.llvm.org/D15120 Files: bindings/python/clang/cindex.py include/clang-c/Index.h

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rsmith added inline comments. Comment at: include/clang/Driver/Options.td:1463 @@ +1462,3 @@ +Group; +def mnofloat128 : Flag<["-"], "mno-float128">, +Group; Per the mangling rules at the top of this file, this should be named `mno_float128`.

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/Sema/SemaExpr.cpp:1156-1159 @@ +1155,6 @@ + + QualType LHSElemType = dyn_cast(LHSType) ? +cast(LHSType)->getElementType() : LHSType; + QualType RHSElemType = dyn_cast(RHSType) ? +

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

On Wed, Feb 3, 2016 at 3:52 PM, Hubert Tong < hubert.reinterpretc...@gmail.com> wrote: > hubert.reinterpretcast added inline comments. > > > Comment at: lib/Sema/SemaExpr.cpp:1156-1159 > @@ +1155,6 @@ > + > + QualType LHSElemType = dyn_cast(LHSType) ? > +

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#340515, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#337144, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#337975, @hubert.reinterpretcast wrote: > In http://reviews.llvm.org/D15120#337654, @rjmccall wrote: > > > In http://reviews.llvm.org/D15120#337552, @hubert.reinterpretcast wrote: > > > > > It remains that the present standardization

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#337384, @rjmccall wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#337384, @rjmccall wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#337552, @hubert.reinterpretcast wrote: > In http://reviews.llvm.org/D15120#337384, @rjmccall wrote: > > > I think it's not unlikely that float128_t will see future standardization > > (as an optional extension, of course), and it would

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#337144, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#337144, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#336891, @rjmccall wrote: > > 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 >

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#336996, @hubert.reinterpretcast wrote: > In http://reviews.llvm.org/D15120#336891, @rjmccall wrote: > > > > The C committee decided that "undefined behavior" was what they could > > > agree on for this sort of case. > > > > > > That's

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai updated this revision to Diff 45998. nemanjai added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#336282, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#336430, @rjmccall wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#336776, @hubert.reinterpretcast wrote: > In http://reviews.llvm.org/D15120#336430, @rjmccall wrote: > > > As I understand it, PPC's long-double (~103 bits of precision) is still > > strictly less precise than float128_t (113 bits of

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#336827, @rjmccall wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a comment. In http://reviews.llvm.org/D15120#336855, @hubert.reinterpretcast wrote: > In http://reviews.llvm.org/D15120#336827, @rjmccall wrote: > > > Here's the thing, though: I don't think there's a reasonable language > > solution here besides saying that float128_t has higher

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/AST/ASTContext.cpp:4635 @@ +4634,3 @@ + // If the two sides have Float128Rank and LongDoubleRank and the two types + // have the same representation on this platform, they have equal rank. + if ((LHSR ==

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:385 @@ +384,3 @@ + unsigned getFloat128Width() const { return Float128Width; } + unsigned getFloat128Align() const { return Float128Align; } + const llvm::fltSemantics () const { I

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:550 @@ -548,1 +549,3 @@ case BuiltinType::Double: +// FIXME: For targets where lond double and __float128 have the same size, +// they are currently indistinguishable in the debugger

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. In http://reviews.llvm.org/D15120#310992, @nemanjai wrote: > 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added a comment. 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

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added inline comments. Comment at: include/clang/Basic/TargetInfo.h:384 @@ +383,3 @@ + unsigned getFloat128Width() const { return 128; } + unsigned getFloat128Align() const { return 128; } + const llvm::fltSemantics () const { hubert.reinterpretcast

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

hubert.reinterpretcast added a comment. I would like to see a test: __float128 qf(); long double ldf(); long double ld{qf()}; // error: narrowing __float128 q{ldf()}; // passes Comment at: include/clang/Basic/TargetInfo.h:384 @@ +383,3 @@ + unsigned

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai updated this revision to Diff 42414. nemanjai added a comment. Updated to address review comments. Rather than listing responses and justification for some of the changes, I'll provide comments inline for better readability. Repository: rL LLVM http://reviews.llvm.org/D15120

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

nemanjai added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:2064 @@ +2063,3 @@ +if (getASTContext().getTargetInfo().useFloat128ManglingForLongDouble()) + Out << "U10__float128"; // Match the GCC mangling +else Please refer to

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

rjmccall added a reviewer: akyrtzi. Comment at: lib/AST/ASTContext.cpp:5424 @@ -5410,1 +5423,3 @@ +// FIXME: need to figure out what this is for __float128 +case BuiltinType::Float128: return 'K'; case BuiltinType::NullPtr:return '*'; // like char*

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

> On Dec 7, 2015, at 12:16 AM, John McCall wrote: > > rjmccall added a reviewer: akyrtzi. > > > Comment at: lib/AST/ASTContext.cpp:5424 > @@ -5410,1 +5423,3 @@ > +// FIXME: need to figure out what this is for __float128 > +case

Re: [PATCH] D15120: Add support for __float128 type to be used by targets that support it

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