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

[John McCall via Phabricator via cfe-commits]

rjmccall added subscribers: akyrtzi, benlangmuir.
rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:131
+    Float128,
+    Ibm128
   };
----------------
This is necessary because it's possible to derive this type from a mode attribute?


================
Comment at: clang/include/clang/Basic/TargetInfo.h:680
+  /// Return the mangled code of __ibm128.
+  virtual const char *getIbm128Mangling() const { return "g"; }
+
----------------
nemanjai wrote:
> 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?
Why are these manglings abstracted into TargetInfo in the first place?  Are there targets that are going to use a different mangling for `__ibm128`?


================
Comment at: clang/lib/AST/ASTContext.cpp:6274
+/// __float128 and __ibm128)
+bool ASTContext::areUnorderedFloatingTypes(QualType LHS, QualType RHS) const {
+  auto *LHSComplex = LHS->getAs<ComplexType>();
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5159
         BT->getKind() == BuiltinType::LongDouble ||
+        BT->getKind() == BuiltinType::Ibm128 ||
         (getContext().getTargetInfo().hasFloat128Type() &&
----------------
I hesitate to ask this, but does `__ibm128` form homogeneous aggregates with `double`s?


================
Comment at: clang/lib/Index/USRGeneration.cpp:708
           c = 'd'; break;
+        case BuiltinType::Ibm128: // FIXME: Need separate tag
         case BuiltinType::LongDouble:
----------------
@akyrtzi @benlangmuir We can just add new USR codes for new types, right?


================
Comment at: clang/lib/Sema/Sema.cpp:1842
+        LongDoubleMismatched = true;
+    }
+
----------------
I think this needs a comment explaining what you're checking for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93377