[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:6230
 /// LHS < RHS, return -1.
 int ASTContext::getFloatingTypeOrder(QualType LHS, QualType RHS) const {
   FloatingRank LHSR = getFloatingRank(LHS);
----------------
I think this function should vocally fail when presented with "unordered" cases.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:803
+  case BuiltinType::Ibm128:
     // FIXME: For targets where long double and __float128 have the same size,
     // they are currently indistinguishable in the debugger without some
----------------
Update the comment for `__ibm128`.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:2245
+  case tok::kw___ibm128:
+    DS.SetTypeSpecType(DeclSpec::TST_ibm128, Loc, PrevSpec, DiagID, Policy);
+    break;
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1188-1190
 /// Diagnose attempts to convert between __float128 and long double if
 /// there is no support for such conversion. Helper function of
 /// UsualArithmeticConversions().
----------------
Comment needs update.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1229
+           &llvm::APFloat::PPCDoubleDouble())) ||
+         EitherIbm128;
 }
----------------
Same comment about possible blocking of `double` to `__ibm128` conversions.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1546-1547
 
   // Diagnose attempts to convert between __float128 and long double where
   // such conversions currently can't be handled.
   if (unsupportedTypeConversion(*this, LHSType, RHSType))
----------------
Comment needs update.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1887
+          FromType == S.Context.Ibm128Ty || ToType == S.Context.Ibm128Ty;
+      if ((Float128AndLongDouble &&
+           (&S.Context.getFloatTypeSemantics(S.Context.LongDoubleTy) ==
----------------
This seems to disable conversion from `double` to `__ibm128`?


================
Comment at: clang/lib/Sema/SemaType.cpp:1562-1563
+    if (!S.Context.getTargetInfo().hasIbm128Type() &&
+        !S.getLangOpts().SYCLIsDevice &&
+        !(S.getLangOpts().OpenMP && S.getLangOpts().OpenMPIsDevice))
+      S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported) << "__ibm128";
----------------
Do the SYCL and OpenMP device exceptions to the error really apply for `__ibm128`?


================
Comment at: clang/test/CodeGenCXX/ibm128-declarations.cpp:6
+
+__ibm128 gf;
+static __ibm128 sgf;
----------------
What does the debug info look like?


================
Comment at: clang/test/CodeGenCXX/ibm128-declarations.cpp:24
+};
+
+__ibm128 func4(__ibm128 a, __ibm128 b) {
----------------
Add a function that reads an `__ibm128` from varargs.


================
Comment at: clang/test/CodeGenCXX/ibm128-declarations.cpp:25
+
+__ibm128 func4(__ibm128 a, __ibm128 b) {
+  return a + b;
----------------
There's a lot of missing testing, especially around implicit conversions, narrowing conversions, and the usual arithmetic conversions.



================
Comment at: clang/test/CodeGenCXX/ibm128-declarations.cpp:25
+
+__ibm128 func4(__ibm128 a, __ibm128 b) {
+  return a + b;
----------------
hubert.reinterpretcast wrote:
> There's a lot of missing testing, especially around implicit conversions, narrowing conversions, and the usual arithmetic conversions.
> 
Make this a `constexpr` function and call it from the initializer of a global declared with `consteval`.


================
Comment at: clang/test/CodeGenCXX/ibm128-declarations.cpp:29
+
+template <typename T> struct T1 {
+  T mem1;
----------------
Add a case where `__ibm128` is used as the type of a non-type template parameter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93377/new/

https://reviews.llvm.org/D93377