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

I mean just the fractional bits since the integral will just be the bit width minus fractional.


Repository:
  rC Clang

https://reviews.llvm.org/D46084