Page MenuHomePhabricator

[Clang] Add __ibm128 type to represent ppc_fp128
Needs ReviewPublic

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

Details

Reviewers
nemanjai
rjmccall
rsmith
hubert.reinterpretcast
jdoerfert
akyrtzi
Group Reviewers
Restricted Project
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 ↗(On Diff #312129)

What does the debug info look like?

24 ↗(On Diff #312129)

Add a function that reads an __ibm128 from varargs.

25 ↗(On Diff #312129)

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

25 ↗(On Diff #312129)

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

29 ↗(On Diff #312129)

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

clang/test/CodeGenCXX/ibm128-declarations.cpp
25 ↗(On Diff #312129)

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.