[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added subscribers: echristo, dblaikie, rjmccall.
rsmith added inline comments.


================
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+  "Fixed point types are only allowed in C">;
 
----------------
Diagnostics should not be capitalized. Also, we generally allow conforming C extensions to be used in other languages unless there is a really good reason not to.


================
Comment at: lib/AST/ASTContext.cpp:6261
+    case BuiltinType::ULongAccum:
+      llvm_unreachable("No ObjC encoding for fixed point types");
+
----------------
`llvm_unreachable` should only be used for cases where you believe the situation to be impossible. I would add these cases to the `return ' ';` case above (for `Float16` / `Float128` / `Half`) for now.


================
Comment at: lib/AST/ExprConstant.cpp:7323-7325
+    case BuiltinType::UShortAccum:
+    case BuiltinType::UAccum:
+    case BuiltinType::ULongAccum:
----------------
Have you checked what value GCC uses for these? Using `integer_type_class` for a non-integer value seems very strange to me, but then they probably are *passed* as integers, so maybe it's correct?


================
Comment at: lib/AST/ItaniumMangle.cpp:2552
+  case BuiltinType::ULongAccum:
+    llvm_unreachable("Fixed point types are disabled for c++");
   case BuiltinType::Half:
----------------
Please check what GCC uses to mangle these, and follow suit; if GCC doesn't have a mangling, you can use a vendor mangling (`u6_Accum`) or produce an error for now, but please open an issue at https://github.com/itanium-cxx-abi/cxx-abi/ to pick a real mangling.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:678-681
+  case BuiltinType::UShortAccum:
+  case BuiltinType::UAccum:
+  case BuiltinType::ULongAccum:
     Encoding = llvm::dwarf::DW_ATE_unsigned;
----------------
@echristo @dblaikie Is this appropriate?


================
Comment at: lib/CodeGen/CodeGenTypes.cpp:448
+    case BuiltinType::ULongAccum:
       ResultType = llvm::IntegerType::get(getLLVMContext(),
                                  static_cast<unsigned>(Context.getTypeSize(T)));
----------------
jakehehrlich wrote:
> Add TODO for accum types using a different drawf type.
I think this TODO was added to the wrong file. Should it be in CGDebugInfo?


================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2684
 
   // Types added here must also be added to EmitFundamentalRTTIDescriptors.
   switch (Ty->getKind()) {
----------------
Note this comment :)


================
Comment at: lib/Index/USRGeneration.cpp:691
+        case BuiltinType::ULongAccum:
+          llvm_unreachable("No USR name mangling for fixed point types.");
         case BuiltinType::Float16:
----------------
leonardchan wrote:
> phosek wrote:
> > We need some solution for fixed point types.
> Added character ~ to indicate fixed point type followed by string detailing the type. I have not added a test to it because logically, I do not think we will ever reach that point. This logic is implemented in the VisitType method, which mostly gets called by visitors to c++ nodes like VisitTemplateParameterList, but we have disabled the use of fixed point types in c++. VisitType does get called in VisitFunctionDecl but the function exits early since we are not reading c++ (line lib/Index/USRGeneration.cpp:238).
@rjmccall Is this an acceptable USR encoding? (Is our USR encoding scheme documented anywhere?)


================
Comment at: lib/Sema/SemaType.cpp:1390-1394
+    if (S.getLangOpts().CPlusPlus) {
+      S.Diag(DS.getTypeSpecTypeLoc(), diag::err_fixed_point_only_allowed_in_c);
+      Result = Context.ShortAccumTy;
+      break;
+    }
----------------
As noted earlier, we should not have such a restriction.


================
Comment at: lib/Sema/SemaType.cpp:1424
+        case DeclSpec::TSW_longlong:
+          // TODO: Replace with diag
+          llvm_unreachable("Unable to specify long long as _Accum width");
----------------
No need for a diagnostic as this is already caught elsewhere.


Repository:
  rC Clang

https://reviews.llvm.org/D46084