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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Seems that conversion diagnostic test cases are completely missing.



================
Comment at: clang/include/clang/Basic/TargetInfo.h:680
+  /// Return the mangled code of __ibm128.
+  virtual const char *getIbm128Mangling() const { return "g"; }
+
----------------
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?


================
Comment at: clang/lib/AST/ASTContext.cpp:6204
     case HalfRank: llvm_unreachable("Complex half is not supported");
+    case Ibm128Rank: llvm_unreachable("Complex __ibm128 is not supported");
     case FloatRank:      return FloatComplexTy;
----------------
Why? Is this coming in an upcoming patch or is this something that will never be available?


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


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1229
+           &llvm::APFloat::PPCDoubleDouble())) ||
+         EitherIbm128;
 }
----------------
hubert.reinterpretcast wrote:
> Same comment about possible blocking of `double` to `__ibm128` conversions.
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>)`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93377