[Clang] Enable IC/IF mode for __ibm128

[qiucf]

Summary

As for 128-bit floating points on PPC, compiler should have three machine modes:

• IFmode, Always IBM extended double
• KFmode, Always IEEE 754R 128-bit floating point (implemented in D80374)
• TFmode, Matches the default for long double
  • If -mabi=ieeelongdouble, TFmode is IEEE 754R 128-bit floating point, and the L/l suffixes produces IEEE 754R constants
  • If -mabi=ibmlongdouble, TFmode is IBM extended double, and the L/l suffixes produces IBM extended double constants

[rjmccall]

clang/include/clang/AST/ASTContext.h

44 ↗

(On Diff #373152)

It's a shame to have to do this in such a pervasively-included header as ASTContext.h. Can we make TargetInfo::RealType an enum class at namespace scope, so that we can just forward-declare it in this file?

This would also be a good opportunity to rename the enum to something like ModeTypeKind.

If we do that, it should be a separate patch, so that this can just be the __ibm128 functionality change.

clang/lib/Basic/TargetInfo.cpp

300

__ibm128 is target-specific, right? So this should return NoFloat if someone requests IF or IC and the target doesn't support it, just like we do when the target doesn't support KF.

[qiucf]

Split and rebase upon D111391.

[aaron.ballman]

I'd appreciate some codegen tests for the new functionality as well (so we're testing not just "do we accept it" but also "does it work" as well), but this looks correct to me.

[rjmccall]

I agree. It doesn't have to be a CodeGen test, just anything that directly verifies that we get the right type, since I think those calls can succeed due to promotion.

[qiucf]

Add codegen test to show generated type in IR.

[rjmccall]

Thanks, LGTM

[eandrews]

This has changed the behavior for -

// Define __complex128 type corresponding to __float128 (as in GCC headers).
typedef _Complex float __attribute__((mode(TC))) __complex128;

void check() {
// CHECK: alloca { fp128, fp128 }   <----- Fails because alloca { x86_fp80, x86_fp80 } is generated instead
__complex128 tmp;
}

In TargetInfo.cpp, we used to check long double format to decide whether to return LongDouble or Float128. This patch doesn't, and so the return value has changed from FloatModeKind::Float128 to clang::FloatModeKind::LongDouble (ExplicitType). Should we be checking the format?

[rjmccall]

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

[eandrews]

In D109950#3097161, @rjmccall wrote:

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

I think the old logic should be preserved yes. However, I'm not sure if this patch exposes an existing bug. LongDoubleFormat here is llvm::semX87DoubleExtended.

In APFloat.cpp, it is defined as -

static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};

i.e. 80 bit size. Is this the right format for complex type specified using mode 'TC'? I am not very familiar with floating point semantics, so I thought I would ask.

[rjmccall]

In D109950#3097544, @eandrews wrote:

In D109950#3097161, @rjmccall wrote:

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

I think the old logic should be preserved yes. However, I'm not sure if this patch exposes an existing bug. LongDoubleFormat here is llvm::semX87DoubleExtended.

In APFloat.cpp, it is defined as -

static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};

i.e. 80 bit size. Is this the right format for complex type specified using mode 'TC'? I am not very familiar with floating point semantics, so I thought I would ask.

The existing logic is that TF and TC should based on long double if the target has a 128-bit long double format, and otherwise they should be based on float128_t if that exists, and otherwise they're invalid. We should definitely not be using long double when it's not a 128-bit format, like if it's the 80-bit x87 format.

I do wonder whether that's the right priority between float128_t and a double-double long double. I think that particular configuration only exists on certain PPC targets; does anyone know what GCC does?

[eandrews]

In D109950#3097652, @rjmccall wrote:

In D109950#3097544, @eandrews wrote:

In D109950#3097161, @rjmccall wrote:

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

I do wonder whether that's the right priority between float128_t and a double-double long double. I think that particular configuration only exists on certain PPC targets; does anyone know what GCC does?

I don't know what GCC does and am not really sure how to go about figuring it out. I've uploaded a patch preserving old logic for review here - https://reviews.llvm.org/D112975. If I should do something different, please let me know. Thank you for your input!

[rjmccall]

Thanks. @hubert.reinterpretcast, @qiucf, can you verify that other compilers for PPC follow the logic for TF / TC that we now have with Elizabeth's patch? There are three different type specifiers (long double, __ibm128, and float128_t) which represent formally distinct types, and IIUC there are a bunch of different flags and target options that change the meaning of long double and/or disable/enable __ibm128 and float128_t. We care about exactly which type is produced by the mode attribute, so you'll have to do something which is sensitive to the exact formal type, like _Generic or a C++ template or doing a pointer conversion without a cast; checking code generation will only tell us the underlying format.

[qiucf]

In D109950#3101740, @rjmccall wrote:

Thanks. @hubert.reinterpretcast, @qiucf, can you verify that other compilers for PPC follow the logic for TF / TC that we now have with Elizabeth's patch? There are three different type specifiers (long double, __ibm128, and float128_t) which represent formally distinct types, and IIUC there are a bunch of different flags and target options that change the meaning of long double and/or disable/enable __ibm128 and float128_t. We care about exactly which type is produced by the mode attribute, so you'll have to do something which is sensitive to the exact formal type, like _Generic or a C++ template or doing a pointer conversion without a cast; checking code generation will only tell us the underlying format.

typedef float __attribute__((mode(TF))) __tf128;
typedef float __attribute__((mode(IF))) __if128;

void foo() { printf(__func__); }
void bar() { printf(__func__); }
#define TEST(x) _Generic(x, __ibm128: foo, default: bar)()

int main() {
  __if128 x;
  __tf128 y;
  TEST(x);
  TEST(y);
}

// GCC (long double=ieee): foobar
// GCC (long double=ibm): foofoo
// Clang (both): foobar

As discussed in https://reviews.llvm.org/D93377#inline-874356 , GCC and Clang handles them differently: in GCC __ibm128 or __float128 is alias to long double under appropriate option, but Clang sees them as different types. While for explicit mode, Clang thinks float __attribute__((mode(IF))) and __ibm128 are the same type. The patch doesn't change the behavior.

For x86 __complex128 check(__complex128 a, __complex128 b) { return a+b; }, GCC also generates two ___addtf3.

clang/lib/Basic/TargetInfo.cpp

       return FloatModeKind::LongDouble;
     break;
   case 128:
     // The caller explicitly asked for an IEEE compliant type but we still
     // have to check if the target supports it.
     if (ExplicitType == FloatModeKind::Float128)
       return hasFloat128Type() ? FloatModeKind::Float128
                                : FloatModeKind::NoFloat;
-    if (&getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble() ||
-        &getLongDoubleFormat() == &llvm::APFloat::IEEEquad())
-      return FloatModeKind::LongDouble;
-    if (hasFloat128Type())
-      return FloatModeKind::Float128;
+    if (ExplicitType == FloatModeKind::Ibm128)
+      return hasIbm128Type() ? FloatModeKind::Ibm128
+                             : FloatModeKind::NoFloat;
+    if (ExplicitType == FloatModeKind::LongDouble)
+      return ExplicitType;
     break;
   }
 
   return FloatModeKind::NoFloat;
 }
 
 /// getTypeAlign - Return the alignment (in bits) of the specified integer type
 /// enum. For example, SignedInt -> getIntAlign().

clang/lib/Sema/SemaDeclAttr.cpp

     case 'K': // KFmode - IEEE quad precision (__float128)
       ExplicitType = FloatModeKind::Float128;
       DestWidth = Str[1] == 'I' ? 0 : 128;
       break;
     case 'T':
       ExplicitType = FloatModeKind::LongDouble;
       DestWidth = 128;
       break;
+    case 'I':
+      ExplicitType = FloatModeKind::Ibm128;
+      DestWidth = Str[1] == 'I' ? 0 : 128;
+      break;
     }
     if (Str[1] == 'F') {
       IntegerMode = false;
     } else if (Str[1] == 'C') {
       IntegerMode = false;
       ComplexMode = true;
     } else if (Str[1] != 'I') {
       DestWidth = 0;

clang/test/Sema/attr-mode.c

 typedef          float f128ibm __attribute__ ((mode (TF)));
 #elif TEST_64BIT_PPC64
 typedef          float f128ibm __attribute__ ((mode (TF)));
 typedef _Complex float c128ibm __attribute__ ((mode (TC)));
 void f_ft128_arg(long double *x);
 void f_ft128_complex_arg(_Complex long double *x);
 void test_TFtype(f128ibm *a) { f_ft128_arg (a); }
 void test_TCtype(c128ibm *a) { f_ft128_complex_arg (a); }
+typedef float w128ibm __attribute__((mode(IF)));
+typedef _Complex float cw128ibm __attribute__((mode(IC)));
+void f_ibm128_arg(__ibm128 *x);
+void f_ibm128_complex_arg(_Complex __ibm128 *x);
+void test_IFtype(w128ibm *a) { f_ibm128_arg(a); }
+void test_ICtype(cw128ibm *a) { f_ibm128_complex_arg(a); }
 #elif TEST_F128_PPC64
 typedef int invalid_7 __attribute((mode(KF))); // expected-error{{type of machine mode does not match type of base type}}
 typedef int invalid_8 __attribute((mode(KI))); // expected-error{{unknown machine mode}}
 typedef _Complex float cf128 __attribute__((mode(KC)));
 typedef float f128 __attribute__((mode(KF)));
 void f_f128_arg(__float128 *x);
 void f_f128_complex_arg(_Complex __float128 *x);
 void test_KFtype(f128 *a) { f_f128_arg(a); }