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

Leonard Chan via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 23 10:39:02 PDT 2018


leonardchan added a subscriber: sammccall.
leonardchan 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">;
 
----------------
rsmith wrote:
> 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.
We decided not to allow fixed point types in other languages because there is no specification provided in N1169 for addressing some features in other languages. Using C++ as an example, N1169 does not provide recommended characters when name mangling so we do not allow this in C++.


================
Comment at: lib/AST/ItaniumMangle.cpp:2552
+  case BuiltinType::ULongAccum:
+    llvm_unreachable("Fixed point types are disabled for c++");
   case BuiltinType::Half:
----------------
rsmith wrote:
> 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.
It seems that GCC uses the characters for each fixed point type's corresponding integral type (https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/mangle.c). Will follow up on this if we end up enabling fixed point types for C++.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:678-681
+  case BuiltinType::UShortAccum:
+  case BuiltinType::UAccum:
+  case BuiltinType::ULongAccum:
     Encoding = llvm::dwarf::DW_ATE_unsigned;
----------------
rsmith wrote:
> @echristo @dblaikie Is this appropriate?
My bad, this should be changed to `DW_ATE_signed_fixed` and `DW_ATE_unsigned_fixed`


================
Comment at: lib/Index/USRGeneration.cpp:691
+        case BuiltinType::ULongAccum:
+          llvm_unreachable("No USR name mangling for fixed point types.");
         case BuiltinType::Float16:
----------------
rsmith wrote:
> 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?)
I chatted with @sammccall about this who said it was ok to add these types if no one opposed this. I posted this on cfe-dev also and it seemed that no one spoke up about it, so I thought this was ok.

I also couldn't find any standard or documentation about reserving characters for USR. It doesn't seem that USR is also parsed in any way, so I don't think I'm breaking anything (running ninja check-all passes).

And unless we also do enable this for C++, this code actually may not be run since this method will not be visited if limited to C.


Repository:
  rC Clang

https://reviews.llvm.org/D46084





More information about the cfe-commits mailing list