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

[Qiu Chaofan via Phabricator via cfe-commits]

qiucf added inline comments.


================
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;
----------------
nemanjai wrote:
> Why? Is this coming in an upcoming patch or is this something that will never be available?
Sure, I plan to add it in next patch


================
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:
> nemanjai wrote:
> > 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)?
> 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.
> 
Adding assertion here makes `unsupportedTypeConversion` always fail in such cases.


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


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1229
+           &llvm::APFloat::PPCDoubleDouble())) ||
+         EitherIbm128;
 }
----------------
nemanjai wrote:
> 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>)`
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 `fpext`ed into `fp128`.


================
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";
----------------
hubert.reinterpretcast wrote:
> Do the SYCL and OpenMP device exceptions to the error really apply for `__ibm128`?
If host supports `__ibm128` but device does not?


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

https://reviews.llvm.org/D93377