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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum                      , KEYALL)
+
----------------
rsmith wrote:
> 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.
That's the goal. Just disabled in c++ for now with the end goal of getting c++ support. Right now I'm waiting for these types to be added in the next Itanium ABI, which I believe you responded to on Github. Afterwards I'll need to request Microsoft mangling.


================
Comment at: lib/Basic/TargetInfo.cpp:45
+  AccumWidth = AccumAlign = 32;
+  LongAccumWidth = LongAccumAlign = 64;
   SuitableAlign = 64;
----------------
rsmith wrote:
> 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.
The integral and fractional bits will be set in the target and is available in a later patch.


================
Comment at: lib/Basic/TargetInfo.cpp:45
+  AccumWidth = AccumAlign = 32;
+  LongAccumWidth = LongAccumAlign = 64;
   SuitableAlign = 64;
----------------
ebevhan wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > 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.
> > The integral and fractional bits will be set in the target and is available in a later patch.
> > We'll also need the target to inform us of the number of integer and fractional bits for each such type.
> 
> I believe the only one that is needed is for the number of fractional bits; the number of integer bits is implied by the difference between the type width and fractional bits. I think I mention this in one of the other patches.
> 
> 
You're right. I was stuck in the mindset that we would be providing an integral and fractional value.


Repository:
  rC Clang

https://reviews.llvm.org/D46084