This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add __ibm128 type to represent ppc_fp128
ClosedPublic

Authored by qiucf on Dec 15 2020, 11:54 PM.

Details

Summary

Currently, we have no front-end type for ppc_fp128 type in IR. PowerPC target generates ppc_fp128 type from long double now, but there's option (-mabi=(ieee|ibm)longdouble) to control it and we're going to do transition from IBM extended double-double ppc_fp128 to IEEE fp128 in the future.

This patch adds type __ibm128 which always represents ppc_fp128 in IR, as what GCC did for that type. Without this type in Clang, compilation will fail if compiling against future version of libstdcxx (which uses __ibm128 in headers).

Although all operations in backend for __ibm128 is done by software, only PowerPC enables support for it.

There's something not implemented in this patch and can be done in future ones:

  1. Literal suffix for __ibm128 type. w/W is suitable as GCC documented (https://gcc.gnu.org/onlinedocs/gcc/Floating-Types.html) but I believe they haven't implemented it yet.
  2. __attribute__((mode(IF))) should be for __ibm128.

Diff Detail

Event Timeline

qiucf created this revision.Dec 15 2020, 11:54 PM
qiucf requested review of this revision.Dec 15 2020, 11:54 PM
Herald added a project: Restricted Project. · View Herald Transcript
steven.zhang added inline comments.Dec 16 2020, 1:08 AM
clang/lib/AST/ItaniumMangle.cpp
2668

This is a bit confusing as the 'g' is for both float128 and ibm128. I know that PowerPC will mangle the float128 as "u9ieee128", and "g" for ibm128. But c++filt demangle the "g" as float128 which seems not right.

clang/lib/AST/ASTContext.cpp
6230

I think this function should vocally fail when presented with "unordered" cases.

clang/lib/CodeGen/CGDebugInfo.cpp
803

Update the comment for __ibm128.

clang/lib/Parse/ParseExprCXX.cpp
2245

Not sure what the best method is to implement this, but long double and __ibm128 are the same type for GCC when -mabi=ibmlongdouble is in effect.

clang/lib/Sema/SemaExpr.cpp
1188–1190

Comment needs update.

1229

Same comment about possible blocking of double to __ibm128 conversions.

1546–1547

Comment needs update.

clang/lib/Sema/SemaOverload.cpp
1887

This seems to disable conversion from double to __ibm128?

clang/lib/Sema/SemaType.cpp
1562–1563

Do the SYCL and OpenMP device exceptions to the error really apply for __ibm128?

clang/test/CodeGenCXX/ibm128-declarations.cpp
6

What does the debug info look like?

24

Add a function that reads an __ibm128 from varargs.

25

There's a lot of missing testing, especially around implicit conversions, narrowing conversions, and the usual arithmetic conversions.

25

Make this a constexpr function and call it from the initializer of a global declared with consteval.

29

Add a case where __ibm128 is used as the type of a non-type template parameter.

clang/test/CodeGenCXX/ibm128-declarations.cpp
25

Make this a constexpr function and call it from the initializer of a global declared with consteval.

Sorry, I meant constinit and not consteval.

Seems that conversion diagnostic test cases are completely missing.

clang/include/clang/Basic/TargetInfo.h
680

The same mangling for both of these types seems incorrect as Steven pointed out. Why not do something similar to what was done for bfloat16 (i.e. an llvm_unreachable) and override it for PowerPC?

clang/lib/AST/ASTContext.cpp
6204

Why? Is this coming in an upcoming patch or is this something that will never be available?

6230

But is it possible to emit errors here or should there be code explicitly added to Sema to disallow conversions between __ibm128 and __float128 (and long double to/from either of those to which it is not equivalent)?

clang/lib/Sema/SemaExpr.cpp
1229

I am not sure what "same comment" refers to, but I agree that this seems very wrong - doesn't this mean that we can't convert between __ibm128 and any other floating point type?
In any case, I think this can be represented quite concisely as something like:
if (<types have same size> && <types do not have same fltSemantics>)

clang/lib/Sema/SemaOverload.cpp
1887

Oh, now I understand the "same comment" comment above. Yes, this appears to also be a case where we don't want to allow types of the same width but different representation.

akyrtzi resigned from this revision.Dec 21 2020, 7:50 PM
clang/lib/AST/ASTContext.cpp
6230

I did not mean a user-facing error message. I meant that there should be some form of assertion or internal diagnostic here. I believe assert is appropriate.

I will note that this is a public member function of ASTContext. Regardless of explicit code in Sema that does what you describe, I think this function should not present an interface where it does not report "unordered" cases as unordered.

qiucf updated this revision to Diff 313835.Dec 28 2020, 1:12 AM
qiucf marked 14 inline comments as done.

Address comments and add tests

clang/lib/AST/ASTContext.cpp
6204

Sure, I plan to add it in next patch

6230

Adding assertion here makes unsupportedTypeConversion always fail in such cases.

clang/lib/Parse/ParseExprCXX.cpp
2245

Seems clang is also different from GCC under -mabi=ieeelongdouble? I saw __float128 and long double are the same for GCC but not for clang.

clang/lib/Sema/SemaExpr.cpp
1229

I found this would break existing tests of x86_64: x86_fp80 is 96 bits long on x86_32, 128 bits long on x86_64. However tests expect x86_fp80 can be fpexted into fp128.

clang/lib/Sema/SemaType.cpp
1562–1563

If host supports __ibm128 but device does not?

clang/lib/AST/ASTContext.cpp
6230

Yes, I know. I think unsupportedTypeConversion should avoid calling this function when it is possibly dealing with an "unordered" case unless if this function has a way of indicating "unordered" as a result. If there is a helper function for testing the unordered case in this class, then I think unsupportedTypeConversion can simply say that all ordered cases are supported (before doing more to figure out the unordered cases that are safe).

clang/lib/Parse/ParseExprCXX.cpp
2245

Have you checked whether the new libstdc++ for which this support is being added needs the GCC behaviour to work properly?

The GCC behaviour allows the following to be compiled without introducing novel overload resolution tiebreakers:

void f(__float128);
void f(__ibm128);
void f(int);

long double ld;

int main() { f(ld); }
clang/lib/Sema/SemaType.cpp
1562–1563

Can you add a test that makes use of this? Also, there should be a case that triggers err_device_unsupported_type.

Are you committed to the name __ibm128? I think a name that says something about floating point would be better than just a company name and a number. "double double" is the common name for this format pretty much everywhere, and while I certainly would *not* suggest actually spelling it double double (i.e. with repeated type specifiers0), you could certainly use something like __doubledouble.

Are you committed to the name __ibm128?

Insofar as that's what GCC on Power (for example, gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 from 2017) has shipped with for several years, yes.

Are you committed to the name __ibm128?

Insofar as that's what GCC on Power (for example, gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 from 2017) has shipped with for several years, yes.

Okay, it wasn't clear from the description that this was an already-shipping type. In that case, it's regrettable but obviously just something we have to match.

jwakely added a subscriber: jwakely.Mar 2 2021, 9:10 AM
qiucf updated this revision to Diff 327716.Mar 3 2021, 1:50 AM
qiucf marked 2 inline comments as done.
  • Add helper method to count 'unordered' cases
  • Add missing tests (in last update)
  • Add test for openmp device-host case
qiucf added inline comments.Mar 3 2021, 10:33 PM
clang/lib/Parse/ParseExprCXX.cpp
2245

As I saw both GCC and clang have error for ambiguous operator<< for:

std::cout << "long double:\n";
std::cout << std::numeric_limits<long double>::max() << std::endl;
std::cout << std::numeric_limits<long double>::min() << std::endl;

std::cout << "__float128:\n";
std::cout << std::numeric_limits<__float128>::max() << std::endl;
std::cout << std::numeric_limits<__float128>::min() << std::endl;

std::cout << "__ibm128:\n";
std::cout << std::numeric_limits<__ibm128>::max() << std::endl;
std::cout << std::numeric_limits<__ibm128>::min() << std::endl;
clang/lib/Parse/ParseExprCXX.cpp
2245

@jwakely, are the overload resolution errors expected? @qiucf, are you sure you have a sufficiently new libstdc++?

jwakely added inline comments.Mar 12 2021, 11:58 PM
clang/lib/Parse/ParseExprCXX.cpp
2245

@jwakely, are the overload resolution errors expected?

Yes. Iostreams support long double but not __float128, unless that happens to be the same type as long double (due to a -mabi=ieeelongdouble option).

clang/lib/Parse/ParseExprCXX.cpp
2245

Meaning that Clang's __float128 iosteams support (with libstdc++) would diverge from GCC.

For example, Clang reports the call below as ambiguous even with -mabi=ieeelongdouble:

void f(double);
void f(long double);

void g(__float128 f128) { f(f128); }

https://godbolt.org/z/dhYEKa

Insofar as the user experience goes, is this lack of iostreams support for __float128 (even with -mabi=ieeelongdouble) within the realm of the intended design of libstdc++?

jwakely added inline comments.Mar 13 2021, 11:18 AM
clang/lib/Parse/ParseExprCXX.cpp
2245

The lack of iostreams support for __float128 is the intended design.

On power we support float, double and three types of long double. If __float128 is a distinct type from all those long double types it won't work.

GCC on power defines __float128 as a macro expanding to __ieee128, so it is the same as one of the long double types.

clang/lib/Parse/ParseExprCXX.cpp
2245

Okay, it sounds like the different treatment Clang has does not really interfere with recommended usage insofar as libstdc++ iostreams (and hopefully this extends to the rest of the library).

clang/bindings/python/clang/cindex.py
2061–2062

This looks suspiciously like the result of this file having not been maintained for the additions of:

CXType_Float16 = 32,
CXType_ShortAccum = 33,
CXType_Accum = 34,
CXType_LongAccum = 35,
CXType_UShortAccum = 36,
CXType_UAccum = 37,
CXType_ULongAccum = 38,
CXType_BFloat16 = 39,

What test coverage fails if the addition of TypeKind.IBM128 is omitted from this patch?

clang/include/clang-c/Index.h
3283

Looks like CXType_Ibm128 is the "last" built-in type now?

qiucf updated this revision to Diff 330560.Mar 14 2021, 10:31 PM
qiucf marked an inline comment as done.

Update cindex

clang/bindings/python/clang/cindex.py
2061–2062

Change this to 40 for consistency.

Actually I did not get any failure of check-clang even when removing all these floating point types assignment. Test coverage may miss here.

jwakely added inline comments.Mar 23 2021, 4:06 AM
clang/lib/AST/ItaniumMangle.cpp
2668

The (de)mangling is very confusing, but that's consistent with gcc.
See comment 3 and 4 at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98389#c3

I think double-double should never have used g it should have been something else (e.g. u8__ibm128), and then we could have g for the __ieee128 (aka __float128) type. But it is many years too late to change that now.

rjmccall added inline comments.Mar 23 2021, 1:57 PM
clang/lib/AST/ItaniumMangle.cpp
2668

Yeah, I think that's an unfortunate but ultimately understandable platform decision.

qiucf added a comment.Apr 6 2021, 7:08 PM

Ping.. Any further comments?

rjmccall added inline comments.
clang/include/clang/Basic/TargetInfo.h
131

This is necessary because it's possible to derive this type from a mode attribute?

680

Why are these manglings abstracted into TargetInfo in the first place? Are there targets that are going to use a different mangling for __ibm128?

clang/lib/AST/ASTContext.cpp
6274

Is it language semantics that these are unordered or just an implementation limitation? I know WG14 is working on a draft for extended types that allows pairs to not be ordered, but IIRC that's supposed to be based on whether types have a subset relationship. I believe the only FP formats that pairwise don't have subset relationships are bfloat/half and ibm128/fp80, and we don't support ibm128 on any x86 targets so the latter is impossible in practice. The reason we have this restriction on converting between ibm128 and float128 is just that we haven't implemented the conversion in the backend / compiler-rt. I don't think we should lock that limitation in as an actual unordered-type relationship.

clang/lib/CodeGen/TargetInfo.cpp
5159

I hesitate to ask this, but does __ibm128 form homogeneous aggregates with doubles?

clang/lib/Index/USRGeneration.cpp
708 ↗(On Diff #330560)

@akyrtzi @benlangmuir We can just add new USR codes for new types, right?

clang/lib/Sema/Sema.cpp
1842 ↗(On Diff #330560)

I think this needs a comment explaining what you're checking for.

akyrtzi added inline comments.Apr 23 2021, 8:56 AM
clang/lib/Index/USRGeneration.cpp
708 ↗(On Diff #330560)

Yes, it would be good to do that.

qiucf marked 2 inline comments as done.Aug 26 2021, 7:09 PM
qiucf added inline comments.
clang/include/clang/Basic/TargetInfo.h
131

Yes, __attribute__((mode(IF)))

clang/lib/CodeGen/TargetInfo.cpp
5159

Homogeneous floating-point aggregates can have up to four IBM EXTENDED PRECISION members, four IEEE BINARY 128 QUADRUPLE PRECISION members, four _Decimal128 members, or eight members of other floating-point types. (Unions are treated as their largest member. For homogeneous unions, different union alternatives may have different sizes, provided that all union members are homogeneous with respect to each other.) They are passed in floating-point registers if parameters of that type would be passed in floating-point registers. They are passed in vector registers if parameters of that type would be passed in vector registers. They are passed as if each member was specified as a separate parameter.

Yes.

rjmccall added inline comments.Aug 26 2021, 7:24 PM
clang/include/clang/Basic/TargetInfo.h
131

Okay. I guess I agree that it makes sense to do that in a follow-up patch since it requires threading some extra state around.

clang/lib/CodeGen/TargetInfo.cpp
5159

No, I mean, would an __ibm128 in a struct with a pair of doubles be treated as if it were 4 doubles, or is it considered non-homogeneous the same way that a struct with e.g. 4 floats and 2 doubles would be non-homogeneous?

qiucf updated this revision to Diff 369045.Aug 27 2021, 2:01 AM
qiucf marked 3 inline comments as done.
qiucf added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
5159

Ah, I think not. __ibm128 (PowerPC long double also produces ppc_fp128) is distinct type in determining homogeneous aggregates.

rjmccall accepted this revision.Sep 1 2021, 10:38 PM

Okay, thanks. LGTM, then.

This revision is now accepted and ready to land.Sep 1 2021, 10:38 PM
This revision was landed with ongoing or failed builds.Sep 6 2021, 3:02 AM
This revision was automatically updated to reflect the committed changes.