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

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

rsmith accepted this revision.
rsmith added a comment.

Just minor comments. Feel free to commit after handling them.



================
Comment at: include/clang/Basic/LangOptions.def:301
 
+LANGOPT(FixedPointEnabled, 1, 0, "Fixed point types are enabled")
+
----------------
ebevhan wrote:
> Just a minor nit... The 'Enabled' is superfluous.
There's still a superfluous "are enabled" in the description. Please consult the file comment:

```
// The Description field should be a noun phrase, for instance "frobbing all
// widgets" or "C's implicit blintz feature".
```

So just "fixed point types" would be appropriate. (And yes, many of the options in this file get this wrong.)


================
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum                      , KEYALL)
+
----------------
leonardchan wrote:
> jfb wrote:
> > ebevhan wrote:
> > > I believe that having KEYALL will enable the keyword even if -fno-fixed-point is given. Is this desired? It will mean that `_Accum` will not be a valid identifier in standard C regardless of the flag.
> > That seems fine: identifiers starting with underscore and a capital letter already aren't valid identifiers in C and C++ because they're reserved for the implementation.
> I think my test for `-fno-fixed-point` already catches this, but I did not notice until now the `KEYNOCXX` keyword until now. Using this instead allows for not having to check if the language is c++ since `_Accum` is no longer treated as a typename. The corresponding test checking fixed points in c++ has been updated to reflect this.
Just to make sure we're on the same page: it's OK to disable this feature for C++ while you're working on it, but it needs to support C++ by the time you're done.


================
Comment at: include/clang/Driver/Options.td:893
+def fno_fixed_point : Flag<["-"], "fno-fixed-point">, Group<f_Group>,
+                      Flags<[CC1Option]>, HelpText<"Disable fixed point types">;
+
----------------
Only the flag that enables a non-default mode should be a `CC1Option`.


================
Comment at: include/clang/Serialization/ASTBitCodes.h:960
+
+      /// \brief OpenCL image types with auto numeration
+
----------------
Spurious comment, probably from a merge conflict?


================
Comment at: lib/AST/ExprConstant.cpp:7361-7362
+    case BuiltinType::ULongAccum:
+      // GCC does not cover FIXED_POINT_TYPE in its switch stmt and defaults to
+      // no_type_class
+      return GCCTypeClass::None;
----------------
GCC's implementation is irrelevant; please only talk about its observable behavior if you think this deserves a comment.


================
Comment at: lib/AST/ItaniumMangle.cpp:2552
+  case BuiltinType::ULongAccum:
+    llvm_unreachable("Fixed point types are disabled for c++");
   case BuiltinType::Half:
----------------
leonardchan wrote:
> 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++.
OK for now, but as noted, this will need to be addressed eventually.


================
Comment at: lib/Basic/TargetInfo.cpp:45
+  AccumWidth = AccumAlign = 32;
+  LongAccumWidth = LongAccumAlign = 64;
   SuitableAlign = 64;
----------------
jfb wrote:
> This seems weird because Targets which don't have these values for the non-Accum versions will have .e.g. `sizeof(short) != sizeof(short _Accum)`. Is there a point in ever having `_Accum` differ in size, width, and alignment from the underlying type? If not, can you set these values after the sub-target has specified its preferences?
I'm uncomfortable about opting all targets into this behavior with these default values; this will result in an ABI break if later a target updates these to the correct values. A per-target `AccumSupported` flag would help substantially. But this is OK for the short term while you're still working on the feature.

We'll also need the target to inform us of the number of integer and fractional bits for each such type.


================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2684
 
   // Types added here must also be added to EmitFundamentalRTTIDescriptors.
   switch (Ty->getKind()) {
----------------
leonardchan wrote:
> rsmith wrote:
> > Note this comment :)
> Returned false for these types now. Not sure if these types should also be added to EmitFundamentalRTTIDescriptors since the OCL types do not appear under there.
OK, we'll need to remember to revisit this when adding C++ support.


================
Comment at: lib/Sema/SemaType.cpp:1409
+          llvm_unreachable("Unable to specify long long as _Accum width");
+          break;
+      }
----------------
No `break` after `unreachable` -- this is dead code.


================
Comment at: lib/Sema/SemaType.cpp:1424
+          llvm_unreachable("Unable to specify long long as _Accum width");
+          break;
+      }
----------------
Likewise here.


Repository:
  rC Clang

https://reviews.llvm.org/D46084